From 1189f7da987c84d387b47e80732b6867c590bb91 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:37:59 -0700 Subject: [PATCH 01/20] refactor(lint): hold the inline-axis rule with stylelint rather than a test The property list and the value list are stylelint rules inside npm run lint over the same src/**/*.{css,scss} glob, so the guard and its two constants in src/toolchain.test.ts go. --- .stylelintrc.json | 31 ++++++++++++++++++ src/toolchain.test.ts | 74 ++----------------------------------------- 2 files changed, 33 insertions(+), 72 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 3007718..7023d87 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -7,6 +7,37 @@ { "message": "Class selectors are camelCase, so a component reads them back as styles.thisName rather than through a bracketed string" } + ], + "property-disallowed-list": [ + [ + "left", + "right", + "margin-left", + "margin-right", + "padding-left", + "padding-right", + "border-left", + "border-right", + "border-left-color", + "border-left-style", + "border-left-width", + "border-right-color", + "border-right-style", + "border-right-width" + ], + { + "message": "Inline-axis geometry is logical, so one stylesheet serves both reading directions: use inset-inline-start, margin-inline-end, padding-inline or border-inline-end" + } + ], + "declaration-property-value-disallowed-list": [ + { + "text-align": ["/^(?:left|right)$/"], + "float": ["/^(?:left|right)$/"], + "clear": ["/^(?:left|right)$/"] + }, + { + "message": "An inline-axis value follows the document rather than the page: use text-align: start, float: inline-start or clear: inline-end" + } ] } } diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index 7f76492..58c017c 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -492,43 +492,6 @@ function styleSheetParts(source: string): StyleSheetParts { return { declarations, selectors }; } -/** - * The physical properties that name one end of the inline axis, so a sheet - * declaring one serves a left-to-right document and silently mis-serves a - * right-to-left one. - * - * The block axis is deliberately absent. Top and bottom mean the same thing - * whichever way the text runs, so banning them would be churn rather than a - * rule. - */ -const PHYSICAL_INLINE_PROPERTIES: ReadonlySet = new Set([ - "left", - "right", - "margin-left", - "margin-right", - "padding-left", - "padding-right", - "border-left", - "border-right", - "border-left-color", - "border-left-style", - "border-left-width", - "border-right-color", - "border-right-style", - "border-right-width", -]); - -/** - * The properties whose value, rather than whose name, can name one end of the - * inline axis. Each has a logical pair, start and end, that follows the - * document instead. - */ -const PHYSICAL_INLINE_VALUED_PROPERTIES: ReadonlySet = new Set([ - "text-align", - "float", - "clear", -]); - /** The component holding the four glyphs that mean a direction. */ const DIRECTIONAL_GLYPH_COMPONENT = "src/components/DataTable/Pagination.tsx"; @@ -2011,41 +1974,8 @@ describe("toolchain baseline", () => { expect(globalSheets("src/a11y.browser.test.tsx")).toEqual(shipped); }); - // Direction-dependent geometry is written once, on the inline axis, so one - // stylesheet serves both directions and there is no second artifact to keep in - // step. Five declarations in this tree were physical and were rewritten; a - // sixth arriving is invisible to every other check here, and is exactly the - // kind of thing that is correct in the browser the author happens to use. - // - // A test rather than a lint rule because the standard configuration in use - // carries no such rule, and the plugin that does is a new dependency for five - // declarations. This file already walks the tree and already parses what it - // asks about, so the guard costs a function rather than an install. - it("keeps direction-dependent geometry on the inline axis in every stylesheet", () => { - const sheets = findStyleSheets(join(projectRoot, "src")); - - expect( - sheets.length, - "src/ carries no stylesheet, so this guard is reading nothing", - ).toBeGreaterThan(0); - - const offenders = sheets.flatMap((sheet) => { - const name = relative(projectRoot, sheet); - - return styleSheetParts(readFileSync(sheet, "utf8")) - .declarations.filter( - ({ property, value }) => - PHYSICAL_INLINE_PROPERTIES.has(property) || - (PHYSICAL_INLINE_VALUED_PROPERTIES.has(property) && - /\b(?:left|right)\b/.test(value)), - ) - .map(({ property, value }) => `${name}: ${property}: ${value}`); - }); - - expect(offenders).toEqual([]); - }); - - // The two ways the rule above is most likely to be undone. Each is otherwise a + // The two ways the inline-axis rule, now `property-disallowed-list` in + // `.stylelintrc.json`, is most likely to be undone. Each is otherwise a // sentence in a plan with nothing behind it. // // The mirror rule that turns the four page glyphs is written on the direction From c6482f112fc0995fd4147107f88d1b95586e18e1 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:39:33 -0700 Subject: [PATCH 02/20] refactor(lint): reject the :dir() pseudo-class with stylelint The rule now covers the global sheet as well as the component sheets, and the hand-written SCSS declaration splitter and both stylesheet walkers go with the guard that needed them. --- .stylelintrc.json | 6 ++ src/toolchain.test.ts | 155 ++---------------------------------------- 2 files changed, 13 insertions(+), 148 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 7023d87..7ff93ca 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -38,6 +38,12 @@ { "message": "An inline-axis value follows the document rather than the page: use text-align: start, float: inline-start or clear: inline-end" } + ], + "selector-disallowed-list": [ + ["/:dir\\(/"], + { + "message": "The :dir() pseudo-class landed in Chrome 120 against this application's floor of 111, so it ships inert in the very browsers the floor exists to name: select the direction on the attribute, as [dir=\"rtl\"] .navButton svg does" + } ] } } diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index 58c017c..e621455 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -371,127 +371,6 @@ function findSourceFiles(directory: string): string[] { return found; } -/** - * Every stylesheet under a directory, both dialects, so a rule asked of the - * styling can be asked of all of it rather than of the dialect that happened to - * be checked. The global sheet is plain CSS and every component sheet is SCSS. - */ -function findStyleSheets(directory: string): string[] { - const found: string[] = []; - - for (const entry of readdirSync(directory, { withFileTypes: true })) { - const path = join(directory, entry.name); - - if (entry.isDirectory()) { - if (SKIPPED_DIRECTORIES.has(entry.name)) continue; - found.push(...findStyleSheets(path)); - } else if (/\.(css|scss)$/.test(entry.name)) { - found.push(path); - } - } - - return found; -} - -/** One declaration as written, with the property and the value already split. */ -interface StyleDeclaration { - readonly property: string; - readonly value: string; -} - -/** A stylesheet reduced to the two constructs the guards below ask about. */ -interface StyleSheetParts { - readonly declarations: readonly StyleDeclaration[]; - readonly selectors: readonly string[]; -} - -/** - * A stylesheet split into its declarations and its selectors. - * - * Constructs rather than raw text, which is this file's standard and is - * load-bearing here twice over. Every sheet in this tree carries paragraphs - * explaining itself, and the rules below are exactly the sort a comment states - * in order to say why it is being obeyed: a text search would go red on the - * explanation as readily as on a violation, at which point the guard gets - * deleted rather than kept. Quoted runs are carried through rather than - * dropped, because a selector matching an attribute value is a quoted run and - * one guard below reads it. - * - * At-rules are not declarations: an include, a use and a media prelude all end - * in a semicolon or open a block, and none of them sets a property. - */ -function styleSheetParts(source: string): StyleSheetParts { - const declarations: StyleDeclaration[] = []; - const selectors: string[] = []; - let buffer = ""; - let index = 0; - - const flushDeclaration = (): void => { - const text = buffer.trim(); - buffer = ""; - - if (text === "" || text.startsWith("@")) return; - - const colon = text.indexOf(":"); - - if (colon === -1) return; - - declarations.push({ - property: text.slice(0, colon).trim().toLowerCase(), - value: text - .slice(colon + 1) - .trim() - .toLowerCase(), - }); - }; - - while (index < source.length) { - const character = source[index]; - const following = source[index + 1]; - - if (character === '"' || character === "'") { - const close = source.indexOf(character, index + 1); - const end = close === -1 ? source.length : close + 1; - - buffer += source.slice(index, end); - index = end; - continue; - } - - if (character === "/" && following === "/") { - const end = source.indexOf("\n", index); - - index = end === -1 ? source.length : end; - continue; - } - - if (character === "/" && following === "*") { - const end = source.indexOf("*/", index + 2); - - index = end === -1 ? source.length : end + 2; - continue; - } - - if (character === "{") { - selectors.push(buffer.trim()); - buffer = ""; - index += 1; - continue; - } - - if (character === "}" || character === ";") { - flushDeclaration(); - index += 1; - continue; - } - - buffer += character; - index += 1; - } - - return { declarations, selectors }; -} - /** The component holding the four glyphs that mean a direction. */ const DIRECTIONAL_GLYPH_COMPONENT = "src/components/DataTable/Pagination.tsx"; @@ -1974,33 +1853,13 @@ describe("toolchain baseline", () => { expect(globalSheets("src/a11y.browser.test.tsx")).toEqual(shipped); }); - // The two ways the inline-axis rule, now `property-disallowed-list` in - // `.stylelintrc.json`, is most likely to be undone. Each is otherwise a - // sentence in a plan with nothing behind it. - // - // The mirror rule that turns the four page glyphs is written on the direction - // attribute for a measured reason: :dir() landed in Chrome 120 and this - // application's floor is 111, so the pseudo-class ships inert in the very - // browsers the floor exists to name. It is also the tidier-looking spelling, - // which is precisely why a later reader substitutes it. - it("selects direction on the attribute rather than on the pseudo-class", () => { - const offenders = findStyleSheets(join(projectRoot, "src")) - .filter((sheet) => sheet.endsWith(".scss")) - .flatMap((sheet) => - styleSheetParts(readFileSync(sheet, "utf8")) - .selectors.filter((selector) => selector.includes(":dir(")) - .map((selector) => `${relative(projectRoot, sheet)}: ${selector}`), - ); - - expect(offenders).toEqual([]); - }); - - // The other substitution: a branch in the component choosing between two glyph - // components on the direction, which is a prop and a coverage line for what one - // declaration does. A returning branch is invisible to the stylesheet guard - // above because it is not CSS, and invisible to the shared layer's literal - // guard because a glyph component is neither a text child nor a string, so it - // is asserted here or nowhere. + // The way the mirror rule is undone in JavaScript rather than in CSS: a branch + // in the component choosing between two glyph components on the direction, + // which is a prop and a coverage line for what one declaration does. It is + // invisible to the stylelint rules that hold the stylesheets because it is not + // CSS, and invisible to the shared layer's literal guard because a glyph + // component is neither a text child nor a string, so it is asserted here or + // nowhere. it("picks the page glyphs with a stylesheet rather than with a branch", () => { const alternatives = jsxAlternatives( moduleSource(DIRECTIONAL_GLYPH_COMPONENT), From 0941607974968731cbcd6af1ac897f135e7f2c82 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:48:05 -0700 Subject: [PATCH 03/20] refactor(lint): hold the shared layer's no-literal rule with eslint react/jsx-no-literals rejects a rendered text child and a no-restricted-syntax selector rejects a string literal on aria-label, title, placeholder or alt. Deletes the hand-rolled JSX literal walker and the guard behind it. --- eslint.config.js | 31 ++++++++++++++ src/toolchain.test.ts | 95 ------------------------------------------- 2 files changed, 31 insertions(+), 95 deletions(-) diff --git a/eslint.config.js b/eslint.config.js index 1524064..04bca61 100755 --- a/eslint.config.js +++ b/eslint.config.js @@ -24,6 +24,22 @@ const TEST_SCAFFOLDING_IMPORT = { "src/test/ holds test scaffolding and is excluded from coverage. Importing it from application code moves executing code outside the coverage gate.", }; +// The four attributes whose string value a reader perceives. `aria-sort` and +// `aria-live` carry strings in that layer too and are deliberately outside the +// set: both take a value the standard defines and assistive technology matches +// on, so translating either would break the feature rather than localize it. +// +// no-restricted-syntax is configured per rule and not per selector, exactly as +// no-restricted-imports is above, so each selector set is declared once here and +// composed per block below. A block that named the rule and forgot a set would +// silently unrestrict it for every file that block matches. +const READER_FACING_ATTRIBUTE = { + selector: + 'JSXAttribute[name.name=/^(aria-label|title|placeholder|alt)$/][value.type="Literal"]', + message: + "src/components/ renders any collection for any reader, so a string here is a claim about which reader. Add the entry to the catalogs and read it off DataTableLabels, PaginationLabels or SearchInputLabels.", +}; + export default defineConfig([ // Build output and test-runner output, not authored source. `eslint .` walks // the working tree, so without this the gate reports parse errors for an @@ -116,6 +132,21 @@ export default defineConfig([ ], }, }, + // The same layer rule one level down from the imports above: a hardcoded + // sentence needs no import, so the two import rules cannot see it. ignoreProps + // is deliberate, because the attribute half is narrower than every prop and is + // the selector beside it. + { + files: ["src/components/**/*.{ts,tsx}"], + ignores: ["src/**/*.test.{ts,tsx}", "src/**/*.test-d.ts"], + rules: { + "react/jsx-no-literals": [ + "error", + { noStrings: true, ignoreProps: true, allowedStrings: [] }, + ], + "no-restricted-syntax": ["error", READER_FACING_ATTRIBUTE], + }, + }, // The type-aware rules need declarations to reason about, and the plain // JavaScript in this tree has none: a build script reading an untyped CSV // parser and a flat config importing a plugin that ships no types. Every diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index e621455..bc41554 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -1058,69 +1058,6 @@ function localeCallSites(file: ts.SourceFile): string[] { return found; } -/** The layer that renders any collection, and so may name none of them. */ -const SHARED_COMPONENT_DIRECTORY = "src/components"; - -/** - * The attributes whose string value a reader perceives. - * - * Two attributes carrying string values in that directory are deliberately not - * here. The sort state attribute takes one of three values the standard itself - * defines, and the live region's politeness setting takes one of two. Both are - * specification tokens rather than copy: assistive technology matches on them, - * so translating either would not localize anything, it would break the feature. - * They are English because the specification is, which is a different fact from - * a component holding a word for a reader. - */ -const READER_FACING_ATTRIBUTES = new Set([ - "aria-label", - "title", - "placeholder", - "alt", -]); - -/** - * Every string a reader could read out of a component: text rendered between - * tags, and a literal on one of the attributes above. - * - * Parsed rather than searched, which is this file's standard and is what makes - * the guard survivable. Every component in that directory carries paragraphs of - * prose explaining itself, and a search would fail on the explanation of the - * rule as readily as on a violation of it, at which point the guard gets - * deleted rather than obeyed. An attribute whose value is an expression is not - * a literal and is not collected: reading a string out of a prop is the shape - * this rule exists to require. - */ -function readerFacingLiterals(file: ts.SourceFile): string[] { - const found: string[] = []; - - const visit = (node: ts.Node): void => { - // Whitespace between elements is text too, so the letter is what separates - // a rendered word from the indentation around it. - if (ts.isJsxText(node) && /\p{L}/u.test(node.text)) { - found.push(node.text.trim()); - } - - if (ts.isJsxAttribute(node)) { - const name = node.name.getText(file); - const value = node.initializer; - - if ( - READER_FACING_ATTRIBUTES.has(name) && - value !== undefined && - ts.isStringLiteralLike(value) - ) { - found.push(`${name}="${value.text}"`); - } - } - - node.forEachChild(visit); - }; - - file.forEachChild(visit); - return found; -} - describe("toolchain baseline", () => { // This guard used to ban a list of names belonging to the runner that was // removed, and nothing else. A second runner arriving with a config of its own @@ -2118,36 +2055,4 @@ describe("toolchain baseline", () => { "the formatter module no longer builds the three cached instances", ).toEqual(["Intl.Collator", "Intl.NumberFormat", "Intl.PluralRules"]); }); - - // The shared component layer renders any collection for any reader, and a - // literal there is a claim about which. The layer already may not import a - // domain type or the locale layer, and both of those are lint rules over - // imports; a hardcoded sentence needs no import and would pass them both. - // Every word that layer shows now arrives in a labels object, and this is - // what keeps the next one arriving the same way: a second locale added to a - // component still holding a literal is a second set of literals. - it("renders no reader-facing literal in the shared component layer", () => { - const components = findSourceFiles( - join(projectRoot, SHARED_COMPONENT_DIRECTORY), - ); - - expect( - components.length, - "the walk found no component, so this guard read nothing", - ).toBeGreaterThan(0); - - const holders = new Map(); - - for (const path of components) { - const name = relative(projectRoot, path).split(sep).join("/"); - const literals = readerFacingLiterals(parse(readFileSync(path, "utf8"))); - - if (literals.length > 0) holders.set(name, literals); - } - - expect( - Object.fromEntries(holders), - "a component under the shared layer carries a string a reader can read", - ).toEqual({}); - }); }); From 72cd38a906dd5676a2383a5ab48b166f83421fa1 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:50:46 -0700 Subject: [PATCH 04/20] refactor(lint): hold the single-Intl-module rule with eslint Two no-restricted-syntax selectors reject an Intl construction and a locale-aware value helper across src/, exempting the formatter module and the tests. The two halves a disallow rule cannot reach, the inline script and the three-instance floor, stay as a reduced guard. --- eslint.config.js | 47 +++++++++++++++++++++++++++++++++- src/toolchain.test.ts | 59 +++++++++++++------------------------------ 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/eslint.config.js b/eslint.config.js index 04bca61..e34250d 100755 --- a/eslint.config.js +++ b/eslint.config.js @@ -40,6 +40,30 @@ const READER_FACING_ATTRIBUTE = { "src/components/ renders any collection for any reader, so a string here is a claim about which reader. Add the entry to the catalogs and read it off DataTableLabels, PaginationLabels or SearchInputLabels.", }; +// Every way a file asks the platform for a locale: a namespace construction, and +// a call to one of the six value-level helpers that read a locale from the +// machine when called with no argument. A fifth surface resolving a locale of its +// own reintroduces the defect the locale layer closed, and does it invisibly on a +// machine whose own preference is the base tag. +// +// Matched on the tree rather than on the text, which matters twice here: a +// namespace named inside a comment is not a call site, and a type annotation +// naming the same constructor is not one either. +const LOCALE_CALL_SITE = [ + { + selector: + ':matches(NewExpression, CallExpression)[callee.object.name="Intl"]', + message: + "src/i18n/format.ts is the one module that builds a platform locale object, so its caches hold one instance per resolved tag. Reach for collatorFor, numberFormatFor or pluralRulesFor instead.", + }, + { + selector: + "CallExpression[callee.property.name=/^(localeCompare|toLocaleString|toLocaleDateString|toLocaleTimeString|toLocaleLowerCase|toLocaleUpperCase)$/]", + message: + "This reads a locale from the machine rather than the one the application resolved, and builds a formatter per call. Take a collator or a formatter from src/i18n/format.ts instead.", + }, +]; + export default defineConfig([ // Build output and test-runner output, not authored source. `eslint .` walks // the working tree, so without this the gate reports parse errors for an @@ -132,6 +156,23 @@ export default defineConfig([ ], }, }, + // The formatter module is exempt because it is the module the rule names. The + // test globs are exempt because a test asserting a formatted string has to + // compute its expectation through the platform rather than type it: the French + // group separator is a narrow no-break space, and a typed literal fails on a + // difference no terminal renders. So the ban is on shipped call sites. + { + files: ["src/**/*.{ts,tsx}"], + ignores: [ + "src/i18n/format.ts", + "src/**/*.test.{ts,tsx}", + "src/**/*.test-d.ts", + "src/test/**", + ], + rules: { + "no-restricted-syntax": ["error", ...LOCALE_CALL_SITE], + }, + }, // The same layer rule one level down from the imports above: a hardcoded // sentence needs no import, so the two import rules cannot see it. ignoreProps // is deliberate, because the attribute half is narrower than every prop and is @@ -144,7 +185,11 @@ export default defineConfig([ "error", { noStrings: true, ignoreProps: true, allowedStrings: [] }, ], - "no-restricted-syntax": ["error", READER_FACING_ATTRIBUTE], + "no-restricted-syntax": [ + "error", + READER_FACING_ATTRIBUTE, + ...LOCALE_CALL_SITE, + ], }, }, // The type-aware rules need declarations to reason about, and the plain diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index bc41554..86d1455 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -2006,51 +2006,28 @@ describe("toolchain baseline", () => { // The application resolves one locale and four surfaces follow it: the // catalog, the document element, the ordering of text and the grouping of // numbers. A fifth surface asking the platform for a locale of its own would - // reintroduce the defect this phase closed, and would do it invisibly, since - // a machine whose own preference is the base tag renders every one of them - // identically. Counting the call sites is the only thing that notices. + // reintroduce the defect the locale layer closed, and would do it invisibly, + // since a machine whose own preference is the base tag renders every one of + // them identically. // - // Test files are excluded, and deliberately. A test asserting a formatted - // string has to compute the expectation through the platform rather than type - // it, because the French group separator is a narrow no-break space and a - // typed literal fails on a difference no terminal renders. So the ban is on - // shipped call sites, not on the name. - it("asks the platform for a locale in exactly one module", () => { - const sources = findSourceFiles(join(projectRoot, "src")); - + // The modules under src/ are held by the no-restricted-syntax rules in + // eslint.config.js. Two halves of that rule are outside what a lint rule can + // reach, and both are here. ESLint does not lint HTML, so the inline script is + // asserted here or nowhere; and a disallow rule cannot say that the formatter + // module still builds anything, so it passes just as happily on a formatter + // module with its caches deleted. + it("asks the platform for a locale only where the lint rule cannot reach", () => { + // The inline script resolves a locale of its own before any module loads. + // It reaches its answer through a literal map rather than through the + // platform, so it should contribute nothing. expect( - sources.length, - "the source walk found no module under src/, so this guard read nothing", - ).toBeGreaterThan(0); - - const holders = new Map(); - - for (const path of sources) { - const name = relative(projectRoot, path).split(sep).join("/"); - const calls = localeCallSites(parse(readFileSync(path, "utf8"))); - - if (calls.length > 0) holders.set(name, calls); - } - - // The inline script resolves a locale of its own before any module loads, - // so it is walked here too. It reaches its answer through a literal map - // rather than through the platform, so it should contribute nothing, and if - // it ever grows a call this is where that shows up. - const stamped = localeCallSites(inlineScript()); - if (stamped.length > 0) holders.set("index.html", stamped); - - expect( - [...holders.keys()].toSorted(), - "something other than the formatter module asks the platform for a locale", - ).toEqual([FORMATTER_MODULE]); + localeCallSites(inlineScript()), + "the inline script in index.html asks the platform for a locale", + ).toEqual([]); - // Without this the assertion above passes just as happily on a formatter - // module that constructs nothing at all, which is the shape this guard - // would take the day someone deleted the caches it exists to protect. expect( - required( - holders.get(FORMATTER_MODULE), - `the call sites in ${FORMATTER_MODULE}`, + localeCallSites( + parse(readFileSync(join(projectRoot, FORMATTER_MODULE), "utf8")), ).toSorted(), "the formatter module no longer builds the three cached instances", ).toEqual(["Intl.Collator", "Intl.NumberFormat", "Intl.PluralRules"]); From 027c2930dc184b57bfdff2cb9bc9534e7e5f09a5 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:53:16 -0700 Subject: [PATCH 05/20] refactor(lint): hold the page-glyph branch rule with eslint Two no-restricted-syntax selectors scoped to Pagination.tsx reject a ternary with a JSX element on both sides and a two-branch if returning one. That block restates all three selector sets, because the rule is configured per rule and a later block replaces an earlier one outright. Deletes the JSX walkers and the guard behind them. --- eslint.config.js | 36 +++++++++++++++++++ src/toolchain.test.ts | 81 ------------------------------------------- 2 files changed, 36 insertions(+), 81 deletions(-) diff --git a/eslint.config.js b/eslint.config.js index e34250d..de1fa74 100755 --- a/eslint.config.js +++ b/eslint.config.js @@ -64,6 +64,26 @@ const LOCALE_CALL_SITE = [ }, ]; +// The way the mirror rule is undone in JavaScript rather than in CSS: a branch +// choosing between two glyph components on the reading direction. It is +// invisible to the stylelint rules that hold the stylesheets, because it is not +// CSS, and invisible to the literal rule above, because a glyph component is +// neither a text child nor a string. +const DIRECTION_BRANCH = [ + { + selector: + "ConditionalExpression[consequent.type=/^JSX(Element|Fragment)$/][alternate.type=/^JSX(Element|Fragment)$/]", + message: + "Two glyph components chosen on a condition are a prop, a branch and a coverage line for what one transform: scaleX(-1) under an attribute selector already does. Mirror in the stylesheet.", + }, + { + selector: + "IfStatement[alternate] ReturnStatement[argument.type=/^JSX(Element|Fragment)$/]", + message: + "Two glyph components returned from two branches are a prop, a branch and a coverage line for what one transform: scaleX(-1) under an attribute selector already does. Mirror in the stylesheet.", + }, +]; + export default defineConfig([ // Build output and test-runner output, not authored source. `eslint .` walks // the working tree, so without this the gate reports parse errors for an @@ -192,6 +212,22 @@ export default defineConfig([ ], }, }, + // The component holding the four glyphs that mean a direction. All three + // selector sets, because no-restricted-syntax is configured per rule: this + // block replaces the two above outright for this file, and naming only the + // direction set would unrestrict the other two for exactly the component most + // likely to regain a literal. + { + files: ["src/components/DataTable/Pagination.tsx"], + rules: { + "no-restricted-syntax": [ + "error", + READER_FACING_ATTRIBUTE, + ...LOCALE_CALL_SITE, + ...DIRECTION_BRANCH, + ], + }, + }, // The type-aware rules need declarations to reason about, and the plain // JavaScript in this tree has none: a build script reading an untyped CSV // parser and a flat config importing a plugin that ships no types. Every diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index 86d1455..eaffbf3 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -371,69 +371,6 @@ function findSourceFiles(directory: string): string[] { return found; } -/** The component holding the four glyphs that mean a direction. */ -const DIRECTIONAL_GLYPH_COMPONENT = "src/components/DataTable/Pagination.tsx"; - -/** Whether a node is JSX, in any of the three shapes the grammar allows. */ -function isJsx(node: ts.Node): boolean { - return ( - ts.isJsxElement(node) || - ts.isJsxSelfClosingElement(node) || - ts.isJsxFragment(node) - ); -} - -/** - * Whether a statement's own return is JSX, ignoring any nested function. - * - * A callback declared inside the branch returns whatever it returns, which is - * not the branch returning it, so the walk stops at a function boundary. - */ -function returnsJsx(node: ts.Node): boolean { - if (ts.isFunctionLike(node)) return false; - - if (ts.isReturnStatement(node)) { - return node.expression !== undefined && isJsx(node.expression); - } - - return ts.forEachChild(node, returnsJsx) ?? false; -} - -/** - * Every conditional in a file that picks between two pieces of JSX: a ternary - * with an element either side, or an if whose two branches each return one. - * - * A guarded render, which is the shape the table already uses, has JSX on one - * side and nothing on the other and is not one of these. - */ -function jsxAlternatives(file: ts.SourceFile): string[] { - const found: string[] = []; - - const visit = (node: ts.Node): void => { - if ( - ts.isConditionalExpression(node) && - isJsx(node.whenTrue) && - isJsx(node.whenFalse) - ) { - found.push(node.condition.getText(file)); - } - - if ( - ts.isIfStatement(node) && - node.elseStatement !== undefined && - returnsJsx(node.thenStatement) && - returnsJsx(node.elseStatement) - ) { - found.push(node.expression.getText(file)); - } - - node.forEachChild(visit); - }; - - file.forEachChild(visit); - return found; -} - const scannedFiles = findTestFiles(projectRoot).filter( (file) => file !== guardFile, ); @@ -1790,24 +1727,6 @@ describe("toolchain baseline", () => { expect(globalSheets("src/a11y.browser.test.tsx")).toEqual(shipped); }); - // The way the mirror rule is undone in JavaScript rather than in CSS: a branch - // in the component choosing between two glyph components on the direction, - // which is a prop and a coverage line for what one declaration does. It is - // invisible to the stylelint rules that hold the stylesheets because it is not - // CSS, and invisible to the shared layer's literal guard because a glyph - // component is neither a text child nor a string, so it is asserted here or - // nowhere. - it("picks the page glyphs with a stylesheet rather than with a branch", () => { - const alternatives = jsxAlternatives( - moduleSource(DIRECTIONAL_GLYPH_COMPONENT), - ); - - expect( - alternatives, - `${DIRECTIONAL_GLYPH_COMPONENT} chooses between two elements on a condition`, - ).toEqual([]); - }); - // The dataset ceiling is written twice on purpose, once where a reader // evaluating this project reads and once where a reader of the code asks the // question. Two copies of one fact is how the provenance account came to have From 3e4524ea8c54ae8e17fb8b3d1c10c089ca3640a9 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:00:37 -0700 Subject: [PATCH 06/20] test(toolchain): drop the guards that restate what the pipeline already fails on The lint gate, the coverage gate, the CI job and the build each already fail on the thing its guard asserted. Their helpers go with them, and the three rules now enforced by nothing say so where they are written. --- playwright.config.ts | 6 +- src/toolchain.test.ts | 176 ------------------------------------------ 2 files changed, 4 insertions(+), 178 deletions(-) diff --git a/playwright.config.ts b/playwright.config.ts index 1212b02..9e7741c 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -6,8 +6,10 @@ import globalSetup from "./e2e/globalSetup"; // all. The hundred percent threshold stays measured over the deterministic // jsdom project alone, which is the project the only command asking for // coverage is scoped to. The other half of that agreement lives in the coverage -// block of vite.config.ts, and the guard that keeps the end-to-end script from -// quietly growing a coverage flag lives in src/toolchain.test.ts. +// block of vite.config.ts. A coverage flag added to this script would emit a +// second report into the directory the static analysis import reads, and +// nothing fails on that flag appearing, so the carve rests on the scripts being +// read rather than on a gate. // // Two alternatives were rejected. Merging this runner's numbers into the one // report the static analysis import reads would cost a hand-rolled protocol diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index eaffbf3..95e4037 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -26,7 +26,6 @@ const guardFile = here.filename; interface Manifest { scripts?: Record; - browserslist?: unknown; [key: string]: unknown; } @@ -384,45 +383,6 @@ function isEndToEndSpec(file: string): boolean { return relative(projectRoot, file).split(sep)[0] === E2E_DIRECTORY; } -/** - * Array.isArray narrows an unknown to any[], which reintroduces the untyped - * value the check was meant to remove. This narrows to unknown[] instead, so - * the elements stay unknown and have to be checked before they are used. - */ -function isUnknownArray(value: unknown): value is unknown[] { - return Array.isArray(value); -} - -/** browserslist wherever it is configured: inline, keyed by env, or in its own file. */ -function browserslistQueries(): unknown[] { - const configured = manifest.browserslist; - - if (isUnknownArray(configured)) return configured; - - if (configured && typeof configured === "object") { - return Object.values(configured as Record).flatMap( - (value) => (isUnknownArray(value) ? value : []), - ); - } - - const rcPath = join(projectRoot, ".browserslistrc"); - if (existsSync(rcPath)) { - return readFileSync(rcPath, "utf8") - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); - } - - return []; -} - -/** - * Queries whose meaning is decided by upstream data rather than by this manifest. - * Any of them lets a browserslist data release move the build output with no commit. - */ -const MOVING_QUERY = - /\b(defaults|last\s+\d+|dead|since\s+\d{4}|unreleased|maintained|current\s+node|node\s+current|extends|supports)\b|%/i; - const FAKES_CLOCK = /\buseFakeTimers\s*\(/; const CONFIGURES_CLOCK = /\bfakeTimers\s*:/; const IMPORTS_USER_EVENT = /from\s+["']@testing-library\/user-event["']/; @@ -464,13 +424,6 @@ const COVERAGE_EXCLUDE_PATTERNS = [ "src/**/*.d.ts", ]; -// The complete coverage include list. One entry, named here for the same -// reason its exclude sibling is written out: narrowing this to a subdirectory -// satisfies a hundred percent by shrinking the gate's input rather than by -// covering the code, and it is the sibling property the exclude guard does not -// reach. -const COVERAGE_INCLUDE_PATTERNS = ["src/**/*.{ts,tsx}"]; - // A suppression comment in any provider's spelling, matched against raw source // because a hint is itself a comment and blanking comments first would make the // guard vacuous. None exists in this tree: the standing convention is that an @@ -1058,61 +1011,6 @@ describe("toolchain baseline", () => { expect(script, "the test script names no project").toMatch(/--project[= ]/); }); - // The guard above covers the script a developer types and neither of the two - // the pipeline runs. Both are asserted as the properties that make them gates - // rather than as one string, for the same reason: adding a flag is free and - // dropping the one that matters is not. - // - // Without --coverage nothing measures coverage, so the threshold is never - // evaluated and coverage/lcov.info is never written, which the Sonar import - // reads as a silent zero rather than as an error. Without the browser project - // named, the browser script fans out to every project and reports the - // deterministic suite a second time as if it were the real-engine one. - // - // The end-to-end script is read the other way round: it must carry no - // coverage flag at all. The answer written beside its config is that this - // runner measures nothing and the hundred percent threshold stays over the - // deterministic project alone, and without this line that answer is a claim - // about intent that nothing checks. The failure it prevents is silent rather - // than loud: a coverage flag added later emits a second report over the same - // directory the static analysis import reads. - // - // Nothing else has to move for that carve, and both reasons are worth stating - // because both stop holding if the end-to-end specs are ever moved under the - // source directory. The coverage block of the build config is untouched, which - // is what keeps the four-pattern exclude set guard green, and the static - // analysis source set is the source directory, which is what keeps the derived - // inclusions guard green. Both hold because the specs live outside it. - it("keeps each pipeline test script carrying the flags its gate needs, and none it must not", () => { - const coverage = manifest.scripts?.["test:coverage"] ?? ""; - - expect(coverage, "the coverage script collects no coverage").toMatch( - /(^|\s)--coverage\b/, - ); - expect(coverage, "the coverage script names no project").toMatch( - /--project[= ]jsdom\b/, - ); - - expect( - manifest.scripts?.["test:browser"] ?? "", - "the browser script does not name the browser project", - ).toMatch(/--project[= ]browser\b/); - - // Read off the script itself rather than off an empty-string fallback. The - // two assertions above get existence for free because they match a flag - // positively and an absent script matches nothing; this one is the inverted - // case, where an absent script satisfies the pattern it is checked against. - // Without the line below, deleting the end-to-end script entirely passes - // the guard that exists to keep it honest. - const endToEnd = manifest.scripts?.["test:e2e"]; - - expect(endToEnd, "the end-to-end script is gone").toBeDefined(); - expect( - endToEnd as string, - "the end-to-end script collects coverage, which writes a second report into the directory the static analysis import reads", - ).not.toMatch(/(^|\s)--coverage\b/); - }); - // A report the upload step cannot collect is skipped, so the pipeline stays // green over an upload carrying nothing. it("keeps every configured report on a path the upload step collects", () => { @@ -1240,15 +1138,6 @@ describe("toolchain baseline", () => { } }); - // Most of the hook rule family is registered at warn rather than error by the - // plugin's own config, exhaustive-deps among them. Without the flag the gate - // exits zero with all of them reported, so neither the pipeline nor the - // pre-commit hook can fail on the rule that guards every dependency array in - // the tree. - it("fails the lint gate on a warning as well as an error", () => { - expect(manifest.scripts?.lint).toContain("--max-warnings 0"); - }); - // Nothing under src/ imports the icon or the manifest. index.html names each // by href and the bundler copies both out of public/ verbatim, so a rename // breaks neither the build nor the type check: it surfaces as a request for a @@ -1406,49 +1295,6 @@ describe("toolchain baseline", () => { }); }); - // browserslist is pinned to explicit versions like the rest of the manifest. A - // shared query such as "defaults" or "last 2 versions" would let upstream data - // releases move the build output without a commit. - it("pins browserslist to explicit versions rather than a moving query", () => { - const queries = browserslistQueries(); - - expect( - queries.length, - "browserslist is configured nowhere", - ).toBeGreaterThan(0); - - for (const query of queries) { - expect(typeof query, `${String(query)} is not a string`).toBe("string"); - expect(query as string, `${String(query)} is a moving query`).not.toMatch( - MOVING_QUERY, - ); - expect( - query as string, - `${String(query)} names no explicit version`, - ).toMatch(/\d/); - } - }); - - // CI runs the format check and so does the hook, which catches drift before it - // becomes a commit rather than after it becomes a push. - it("runs lint and the format check from the pre-commit hook", () => { - const hookPath = join(projectRoot, ".husky", "pre-commit"); - - expect(existsSync(hookPath), ".husky/pre-commit is missing").toBe(true); - - // Judged on live lines only: commenting the commands out and falling through to - // a bare exit disables the hook while leaving every expected string in the file. - const live = readFileSync(hookPath, "utf8") - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); - - expect(live.some((line) => line.includes("npm run lint"))).toBe(true); - expect(live.some((line) => line.includes("npm run format:check"))).toBe( - true, - ); - }); - // A faked clock plus the user input library deadlocks unless the library is told // which clock to advance, and a file that never restores the real clock leaks the // fake one into whatever runs next. Both were found the hard way during the @@ -1618,28 +1464,6 @@ describe("toolchain baseline", () => { expect(patterns?.toSorted()).toEqual(COVERAGE_EXCLUDE_PATTERNS.toSorted()); }); - // The threshold is the single line that turns the number into a gate, and the - // include list decides what the number is measured over. Both sit beside the - // exclude list and neither was guarded, so the gate could be reverted to a - // report, or fitted to a third of the tree, with every other guard green. - // Compared the same way the exclude list is: reordering is not a weakening - // and must not flap, while narrowing, widening or emptying must all fail. - it("keeps the coverage gate at a hundred percent over the whole source tree", () => { - expect( - coverageBlock(), - `${CONFIG_FILE} declares no hundred percent coverage threshold`, - ).toMatch(/thresholds\s*:\s*\{\s*100\s*:\s*true\s*,?\s*\}/); - - const patterns = coveragePatterns("include"); - - expect( - patterns, - `${CONFIG_FILE} declares no coverage include list`, - ).not.toBeNull(); - - expect(patterns?.toSorted()).toEqual(COVERAGE_INCLUDE_PATTERNS.toSorted()); - }); - // Sonar reads a file the coverage report excludes as main source and counts // every line of it as uncovered, which is how the same tree reported 92.9% // there and 98.5% here. The properties file states that the two lists have to From 1cfdcc002ff0c29d190886ce3c90f9dc72a2bc64 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:28:59 -0700 Subject: [PATCH 07/20] test(theme): drop the four contrast pairs the sweep decides The real-engine axe sweep was instrumented to report the colour pairs it actually samples. It decides four of the seventeen by value, so those four leave the token math and thirteen stay, including every non-text pair, which no rule in the shipped set measures at all. --- src/theme/tokens.test.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/theme/tokens.test.ts b/src/theme/tokens.test.ts index 3d40a8f..d05b1e9 100644 --- a/src/theme/tokens.test.ts +++ b/src/theme/tokens.test.ts @@ -329,26 +329,21 @@ function contrastRatio(a: string, b: string): number { * hover fill, only inherited body text does, and the focus ring sits outside the * border box on the parent surface rather than on the fill it surrounds. * + * Four text pairs are absent: axe decides them by value in the real-engine + * sweep, and CONTRAST-OVERLAP.md records that measurement per pair. + * * The two logo rows are measured by choice. The non-text contrast criterion * exempts logos and logotypes outright, so if a future surface change turns * either one red the correct answer is to drop the exempt pair deliberately, * never to lower a threshold to keep it. */ const PAIRS: Array<[string, string, number]> = [ - ["--color-text", "--color-surface", TEXT_CONTRAST_MINIMUM], - ["--color-text", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-text", "--color-surface-hover", TEXT_CONTRAST_MINIMUM], - ["--color-text-muted", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-text-muted", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-accent", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-accent", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-error", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-error", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], - // The selected segment of the theme control: the surface colour laid on the - // accent, which is the one pairing in the app that reads a background token as - // a foreground. Measured here rather than by hand, so a later accent change - // cannot quietly take the control's label below the text threshold. - ["--color-surface", "--color-accent", TEXT_CONTRAST_MINIMUM], ["--color-border-strong", "--color-surface", NON_TEXT_CONTRAST_MINIMUM], [ "--color-border-strong", From 430bc411934a340679dfae397f363fe0791b0d02 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:36:30 -0700 Subject: [PATCH 08/20] refactor(lint): hold the colour-literal rule with stylelint color-no-hex and color-named cover every keyword where the hand-rolled matcher named 12 of roughly 148. The glob is the whole lint scope with src/index.css nulling the hex rule, so the two component stylesheets that are .css keep their coverage. The SCSS-variable and retired-token halves stay in the test, which stylelint has no rule for. --- .stylelintrc.json | 27 +++++++++++++++- src/theme/tokens.test.ts | 70 ++++------------------------------------ 2 files changed, 32 insertions(+), 65 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 7ff93ca..38ac1a4 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -45,5 +45,30 @@ "message": "The :dir() pseudo-class landed in Chrome 120 against this application's floor of 111, so it ships inert in the very browsers the floor exists to name: select the direction on the attribute, as [dir=\"rtl\"] .navButton svg does" } ] - } + }, + "overrides": [ + { + "files": ["src/**/*.{css,scss}"], + "rules": { + "color-no-hex": [ + true, + { + "message": "Colour in a component stylesheet arrives through var(--color-*) and nowhere else, so a theme switch moves every value at once" + } + ], + "color-named": [ + "never", + { + "message": "Colour in a component stylesheet arrives through var(--color-*) and nowhere else; a keyword is as fixed as a hex and flips with no theme" + } + ] + } + }, + { + "files": ["src/index.css"], + "rules": { + "color-no-hex": null + } + } + ] } diff --git a/src/theme/tokens.test.ts b/src/theme/tokens.test.ts index d05b1e9..7a5473e 100644 --- a/src/theme/tokens.test.ts +++ b/src/theme/tokens.test.ts @@ -79,20 +79,6 @@ const RETIRED_TOKENS = [ "--gray-900", ]; -// The four hex lengths CSS accepts, and nothing longer, so an identifier that -// merely starts with hex digits is not mistaken for a colour. -const HEX_COLOR = /#(?:[0-9a-f]{3,4}|[0-9a-f]{6}|[0-9a-f]{8})(?![0-9a-z-])/gi; - -// The other two forms CSS accepts for a fixed colour. A functional notation and -// a named colour are as fixed as a hex is, and neither flips with the theme, so -// a guard that reads hex alone waves both through. The boundaries exclude a -// hyphen, so var(--gray-50) and white-space are read as the identifiers they -// are rather than as colours. -const COLOR_FUNCTION = - /(? - [...source.matchAll(matcher)].map(([literal]) => literal), - ); -} - /** * The outline declarations in a stylesheet whose value cancels the focus ring, * returned whole so the failure message names the declaration that has to go. @@ -587,10 +562,11 @@ describe("the theme script in index.html", () => { }); }); -// A string check rather than a parse: the CSS parser throws outright on the -// inline comments in the table's stylesheet, and a guard written against the one -// module file that happens to have none would look like it worked. -describe("colour in the component stylesheets", () => { +// The halves of the old colour guard stylelint has no rule for: declaring an +// SCSS variable, and naming a token that no longer exists. A string check rather +// than a parse, because the CSS parser throws outright on the inline comments in +// the table's stylesheet. +describe("stray declarations in the component stylesheets", () => { it("finds the stylesheets by walking rather than by a list", () => { expect( componentStylesheets.length, @@ -598,7 +574,7 @@ describe("colour in the component stylesheets", () => { ).toBeGreaterThan(0); }); - it("leaves no colour literal, SCSS variable or retired token in any of them", () => { + it("leaves no SCSS variable or retired token in any of them", () => { const offenders: string[] = []; for (const file of componentStylesheets) { @@ -607,10 +583,6 @@ describe("colour in the component stylesheets", () => { const source = stripComments(readFileSync(file, "utf8")); const name = relative(projectRoot, file); - for (const literal of colourLiterals(source)) { - offenders.push(`${name}: holds the colour literal ${literal}`); - } - for (const variable of source.matchAll(SCSS_VARIABLE)) { offenders.push(`${name}: declares ${variable[0].trim()}`); } @@ -705,36 +677,6 @@ describe("the focus ring", () => { // spelling below is one a real author reaches for and one an earlier revision of // these matchers passed, so the reach is asserted rather than assumed. describe("the reach of the guards", () => { - it("sees a colour literal in every form CSS accepts for one", () => { - for (const declaration of [ - "color: #abc;", - "color: red;", - "background: rgb(1 2 3);", - "border-color: hsl(0 0% 0%);", - "background: transparent;", - "color: color-mix(in oklab, #fff, #000);", - ]) { - expect( - colourLiterals(declaration), - `${declaration} is invisible to the colour guard`, - ).not.toEqual([]); - } - }); - - it("reads a token reference and a property name as neither", () => { - for (const declaration of [ - "color: var(--color-text);", - "background: var(--gray-50);", - "white-space: nowrap;", - "background-color: var(--color-surface);", - ]) { - expect( - colourLiterals(declaration), - `${declaration} is reported as a colour literal`, - ).toEqual([]); - } - }); - it("sees a length authored off the rem scale, whatever unit carries it", () => { for (const declaration of [ "padding: 1.5em;", From 8a6429e5a9990486063fbcb794917e8be3157554 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:39:26 -0700 Subject: [PATCH 09/20] refactor(lint): hold the off-scale length rule with stylelint declaration-property-unit-allowed-list maps the nine spacing properties and font-size to rem. The visually-hidden mixin nulls it by name for its off-screen clip, because a unit list cannot say "a pixel or two", and widening the rule everywhere for one line would be the wrong trade. The global stylesheet keeps its bounded radius count in the test, which a unit list has no way to make. --- .stylelintrc.json | 23 ++++++ src/styles/_visually-hidden.scss | 5 ++ src/theme/tokens.test.ts | 117 +++++-------------------------- 3 files changed, 46 insertions(+), 99 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 38ac1a4..64719c9 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -61,6 +61,23 @@ { "message": "Colour in a component stylesheet arrives through var(--color-*) and nowhere else; a keyword is as fixed as a hex and flips with no theme" } + ], + "declaration-property-unit-allowed-list": [ + { + "margin": ["rem"], + "margin-block": ["rem"], + "margin-inline": ["rem"], + "padding": ["rem"], + "padding-block": ["rem"], + "padding-inline": ["rem"], + "gap": ["rem"], + "row-gap": ["rem"], + "column-gap": ["rem"], + "font-size": ["rem"] + }, + { + "message": "Spacing and type are authored in rem through a token, so the layout follows the reader's browser font-size setting: reach for var(--space-*) or var(--font-size-*)" + } ] } }, @@ -69,6 +86,12 @@ "rules": { "color-no-hex": null } + }, + { + "files": ["src/styles/_visually-hidden.scss"], + "rules": { + "declaration-property-unit-allowed-list": null + } } ] } diff --git a/src/styles/_visually-hidden.scss b/src/styles/_visually-hidden.scss index 22c43a5..fe4150f 100644 --- a/src/styles/_visually-hidden.scss +++ b/src/styles/_visually-hidden.scss @@ -8,6 +8,11 @@ width: 1px; height: 1px; padding: 0; + + // The one px length on a spacing property in the tree, and the reason this + // file nulls declaration-property-unit-allowed-list in .stylelintrc.json. A + // unit allowed-list cannot say "a pixel or two", which is what the negative + // margin is: the classic off-screen clip, not spacing. margin: -1px; overflow: hidden; clip-path: inset(50%); diff --git a/src/theme/tokens.test.ts b/src/theme/tokens.test.ts index 7a5473e..7a7bb43 100644 --- a/src/theme/tokens.test.ts +++ b/src/theme/tokens.test.ts @@ -98,26 +98,18 @@ const OUTLINE_DECLARATION = const RING_CANCELLING_VALUE = /(? Number(magnitude) > PX_HAIRLINE_MAXIMUM) - .map(([length]) => length), - ...[...readable.matchAll(NON_REM_LENGTH)].map(([length]) => length), - ]; -} - /** * The outline declarations in a stylesheet whose value cancels the focus ring, * returned whole so the failure message names the declaration that has to go. @@ -609,28 +573,17 @@ describe("stray declarations in the component stylesheets", () => { }); }); -// The counterpart to the colour guard, and the reason the walk above takes -// every stylesheet rather than the module ones: the rule this keeps is that -// spacing and type are authored in rem through a token, so the layout follows -// the reader's browser font-size setting. A stylesheet written after this file -// inherits the rule by being walked, without anyone restating it. -describe("length in the stylesheets", () => { - it("leaves no px spacing in any component stylesheet", () => { - const offenders: string[] = []; - - for (const file of componentStylesheets) { - for (const length of offScaleLengths(readFileSync(file, "utf8"))) { - offenders.push( - `${relative(projectRoot, file)}: holds ${length}, which is spacing and belongs to a token`, - ); - } - } - - expect(offenders).toEqual([]); - }); - +// The half of the length rule stylelint's unit allowed-list has no way to say. +// It counts rather than forbids, and it reads src/index.css, where the two +// corner radii are px on purpose: growing with the reader's type would only +// distort the shape. +describe("length in the global stylesheet", () => { it("allows the global stylesheet the corner radii and nothing beside them", () => { - const found = offScaleLengths(readFileSync(cssPath, "utf8")); + const found = [ + ...stripComments(readFileSync(cssPath, "utf8")).matchAll(PX_VALUE), + ] + .filter(([, magnitude]) => Number(magnitude) > HAIRLINE_PX) + .map(([length]) => length); expect( found, @@ -677,40 +630,6 @@ describe("the focus ring", () => { // spelling below is one a real author reaches for and one an earlier revision of // these matchers passed, so the reach is asserted rather than assumed. describe("the reach of the guards", () => { - it("sees a length authored off the rem scale, whatever unit carries it", () => { - for (const declaration of [ - "padding: 1.5em;", - "margin: 12pt;", - "width: 2in;", - "gap: 3mm;", - "inline-size: 40ch;", - "margin-top: -1.5em;", - "padding: 24px;", - ]) { - expect( - offScaleLengths(declaration), - `${declaration} is invisible to the length guard`, - ).not.toEqual([]); - } - }); - - it("reads the rem scale, a hairline and a viewport measure as none of that", () => { - for (const declaration of [ - "padding: 1.5rem;", - "gap: 0.25rem;", - "width: 50%;", - "min-height: 100vh;", - "border-bottom: 1px solid var(--color-border);", - "margin: 0;", - "--space-2em: 1rem;", - ]) { - expect( - offScaleLengths(declaration), - `${declaration} is reported as an off-scale length`, - ).toEqual([]); - } - }); - it("sees a suppressed focus ring however it is spelled", () => { for (const declaration of [ "outline: none;", From 91d4b09d8a52cb689b97a53064c49baa3d0d4cb2 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:43:23 -0700 Subject: [PATCH 10/20] refactor(lint): hold the focus-ring suppression rule with stylelint outline and its three longhands join text-align, float and clear in the one top-level declaration-property-value-disallowed-list object. Top level rather than inside the override, because stylelint replaces a same-named rule for the files an override matches rather than merging with it, so an outline-only override would drop the inline-axis keys with the gate still green. The self-testing guard-reach block goes with the matchers it tested. That the ring is drawn stays in the test, which no disallow rule can say. --- .stylelintrc.json | 16 +++++- src/theme/tokens.test.ts | 105 ++++++--------------------------------- 2 files changed, 28 insertions(+), 93 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 64719c9..2d71844 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -33,10 +33,22 @@ { "text-align": ["/^(?:left|right)$/"], "float": ["/^(?:left|right)$/"], - "clear": ["/^(?:left|right)$/"] + "clear": ["/^(?:left|right)$/"], + "outline": [ + "/(? - RING_CANCELLING_VALUE.test( - required(value, "the outline declaration's value"), - ), - ) - .map(([declaration]) => declaration.trim()); -} - /** * Every stylesheet under src/, found by walking rather than by a list, so a * stylesheet added by a later component is covered the day it lands instead of * the day someone remembers to add it here. Every extension rather than the - * module ones alone, because a shared partial and a global file are the two - * places a rule would otherwise be free to break. + * module ones alone, because a shared partial is a place a rule would otherwise + * be free to break. */ function findStylesheets(directory: string): string[] { const found: string[] = []; @@ -159,12 +138,11 @@ function findStylesheets(directory: string): string[] { return found; } -const stylesheets = findStylesheets(join(projectRoot, "src")); - -// The global file declares the hex primitives every other file reaches for -// through a token, so it is the one exemption from the colour half of the guard -// and from nothing else. -const componentStylesheets = stylesheets.filter((file) => file !== cssPath); +// The global file is read on its own terms below, by the two guards written +// against it, so it is held out of the walk rather than exempted inside one. +const componentStylesheets = findStylesheets(join(projectRoot, "src")).filter( + (file) => file !== cssPath, +); /** Every declaration in the file, keyed by selector then by property. */ function readBlocks(): Map> { @@ -605,59 +583,4 @@ describe("the focus ring", () => { "the global focus rule does not draw its outline from the ring token", ).toContain("--color-focus-ring"); }); - - it("is suppressed by no stylesheet", () => { - const offenders: string[] = []; - - // The global stylesheet is walked alongside the rest: a suppression there - // would cancel the rule from the same file that declares it. - for (const file of stylesheets) { - const source = stripComments(readFileSync(file, "utf8")); - - for (const suppression of focusRingSuppressions(source)) { - offenders.push( - `${relative(projectRoot, file)}: cancels the focus ring with ${suppression}`, - ); - } - } - - expect(offenders).toEqual([]); - }); -}); - -// The guards above read a clean tree, which is the one condition under which a -// guard that matches nothing and a guard that works are indistinguishable. Each -// spelling below is one a real author reaches for and one an earlier revision of -// these matchers passed, so the reach is asserted rather than assumed. -describe("the reach of the guards", () => { - it("sees a suppressed focus ring however it is spelled", () => { - for (const declaration of [ - "outline: none;", - "outline: none !important;", - "outline-style: none;", - "a { color: var(--color-text); outline: none }", - "outline: 0 solid transparent;", - "outline-width: 0;", - "outline-width: 0rem;", - "outline-color: transparent;", - ]) { - expect( - focusRingSuppressions(declaration), - `${declaration} is invisible to the focus-ring guard`, - ).not.toEqual([]); - } - }); - - it("reads a drawn ring and an offset as neither", () => { - for (const declaration of [ - "outline: 2px solid var(--color-focus-ring);", - "outline: 0.125rem solid var(--color-focus-ring);", - "outline-offset: 2px;", - ]) { - expect( - focusRingSuppressions(declaration), - `${declaration} is reported as a suppression`, - ).toEqual([]); - } - }); }); From 6b0cf579300d06755a244bd807df35ffe32d388f Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:00:29 -0700 Subject: [PATCH 11/20] docs(table): trim the comments in the component and hook layers Cut the multi-paragraph rationale blocks to the line or two the conventions ask for. The a11y and ponytail prefixed comments, the NoInfer explanation, the single-consumer hazard on useTheme and both reachability records in tableStateUrl are unchanged in substance. No code moved. --- src/components/DataTable/DataTable.tsx | 109 ++++-------------- src/components/DataTable/Pagination.tsx | 33 +----- src/components/DataTable/TableBody.tsx | 10 +- src/components/DataTable/TableHead.tsx | 18 +-- src/components/DataTable/column.ts | 98 +++------------- src/components/DataTable/sortRows.ts | 41 +------ src/components/DataTable/tableState.ts | 85 +++----------- src/components/DataTable/tableStateUrl.ts | 134 ++++------------------ src/components/SearchInput.tsx | 21 +--- src/components/compareRows.ts | 87 ++------------ src/components/paginate.ts | 24 +--- src/hooks/useDebouncedCallback.ts | 46 ++------ src/hooks/useLocale.ts | 26 +---- src/hooks/usePaginatedRows.ts | 9 +- src/hooks/useSortedRows.ts | 10 +- src/hooks/useTheme.ts | 74 +++--------- 16 files changed, 150 insertions(+), 675 deletions(-) diff --git a/src/components/DataTable/DataTable.tsx b/src/components/DataTable/DataTable.tsx index e9e9a3a..1c0f254 100644 --- a/src/components/DataTable/DataTable.tsx +++ b/src/components/DataTable/DataTable.tsx @@ -11,21 +11,8 @@ import styles from "./DataTable.module.scss"; export type { PaginationLabels }; /** - * Every string this table and the controls under it render. - * - * They arrive as a prop because a table that renders any collection is the one - * thing that cannot know what the collection is called, and a shared component - * carrying one collection's nouns would be shared in name only. The same - * argument is what grew the object from the five entries it opened with to every - * word below: a component holding one reader's language is shared in name only - * too. Nothing in this file is a literal a reader sees. - * - * Several entries are functions because they weave a value into a sentence. - * None of them takes a word: a caller handing over an already-composed phrase - * has made the grammatical decision one layer too early, which is exactly the - * defect the two sort entries below were rewritten to remove. The object is - * expected to hold one identity per language, so those closures stay stable - * across renders. + * Every string this table and the controls under it render. The entries that + * weave a value take that value and never a word, which would decide grammar. */ export interface DataTableLabels { /** Shown in place of the whole view until the rows have arrived once. */ @@ -63,16 +50,12 @@ export interface DataTableLabels { export interface DataTableProps { readonly rows: readonly T[]; readonly columns: readonly Column[]; - /** - * Must be injective. It keys the rows for reconciliation and it breaks ties - * between equal values in the sort, so two rows sharing a value here lose - * their identity and their ordering in the same stroke. - */ + /** Must be injective: it keys the rows and breaks ties in the sort. */ readonly getRowId: (row: T) => string; // The id is read off the column array above and only checked here. Without - // the wrapper the compiler would collect a candidate from this prop too and - // union it in, so a misspelt id would widen the union in silence instead of - // failing at the line that holds it. + // the NoInfer wrapper the compiler would collect a candidate from this prop + // too and union it in, so a misspelt id would widen the union in silence + // instead of failing at the line that holds it. readonly state: TableState>; readonly onSortChange: (columnId: NoInfer) => void; readonly onPageChange: (page: number) => void; @@ -80,12 +63,10 @@ export interface DataTableProps { readonly loading: boolean; // False until the underlying collection has arrived at least once. readonly datasetReady: boolean; - // The text of the failure rather than the failure itself. A component tier - // that renders a message cannot narrow an error object, which also means a - // preserved cause has no path to the screen from here. + // The text of the failure rather than the failure itself, so no component + // tier sees a cause. readonly errorMessage: string | null; - // Optional so the table stays usable on its own, without a container to - // re-run the request behind it. + // Optional so the table stays usable without a container behind it. readonly onRetry?: (() => void) | undefined; readonly labels: DataTableLabels; } @@ -96,11 +77,6 @@ export interface DataTableProps { * a11y: the failure arrives after the initial render, so without a live region * a screen reader user is never told the load failed or that a way back is on * offer. alert rather than status because the table it replaces is gone. - * - * A failed dataset load makes every search fail, so the way back belongs in the - * region that already reports it rather than in a second error surface. A - * native button carries the role, the focus, and the keyboard activation on its - * own. */ function ErrorRegion({ message, @@ -123,18 +99,7 @@ function ErrorRegion({ ); } -/** - * What the sort live region says. - * - * A cleared sort and a sort that has never been applied render the same state, - * so the first render has to stay silent while the press that clears a sort - * does not. That is what hasSorted separates. - * - * It separates the sorted case too, not just the cleared one: a sort can arrive - * without anybody pressing anything, because a link can carry one. That is - * still a first render, and a region reporting what the table is rather than - * what just changed announces something that did not happen. - */ +/** A link can carry a sort nobody pressed, so hasSorted gates the first. */ function sortAnnouncement( labels: DataTableLabels, sortDirection: "asc" | "desc" | null, @@ -143,23 +108,14 @@ function sortAnnouncement( ): string { if (!hasSorted) return ""; if (sortDirection && activeLabel) { - // The direction travels as the value it is rather than as a word chosen - // here. The word it turns into is a fact about a language, and the - // suffixed token this replaced was a word in exactly one of them. + // The direction travels as the value it is, not as a word chosen here: + // which word it becomes is a fact about a language. return labels.sortedAnnouncement(activeLabel, sortDirection); } return labels.sortClearedAnnouncement; } -/** - * What the results live region says. - * - * Silent unless there is a settled result to report, because announcing a count - * mid-request would name rows that are about to be replaced. The empty result - * gets a sentence of its own: emptying a region is not an announcement, so a - * search matching nothing would otherwise be indistinguishable from a request - * that never came back. - */ +/** Silent unless settled: a count mid-request names rows about to go. */ function resultsAnnouncement( labels: DataTableLabels, settled: boolean, @@ -171,10 +127,7 @@ function resultsAnnouncement( return labels.results(shown, total); } -/** - * The sort, described for the caption, in the words the caption weaves into a - * sentence rather than the words the live region announces. - */ +/** The sort described for the caption, in the caption's words, not the region's. */ function sortSummary( labels: DataTableLabels, sortDirection: "asc" | "desc" | null, @@ -185,17 +138,9 @@ function sortSummary( } /** - * Renders a collection as a sortable, paginated table. - * - * It holds nothing. The sort column and direction, the page position, the page - * size and the committed query all arrive in one object and leave as three - * calls describing what the user did, so the owner of that object decides what - * the next one is. Both derivations below are memos over modules that know - * nothing about React. - * - * Ordering and every rendered cell are delegated to the column descriptors, and - * every string naming what the rows are comes from the labels object, so - * nothing in this file knows which collection it is showing. + * Renders a collection as a sortable, paginated table, holding nothing itself. + * Cells come from the descriptors and words from the labels, so nothing here + * knows which collection it is showing. */ export function DataTable({ rows, @@ -225,29 +170,21 @@ export function DataTable({ state.pageSize, ); - // The announcements name what is on screen, and what is on screen is the - // column label. state.sortColumnId is the descriptor's id, which is not the name of - // anything the reader can see. - // Empty rather than absent when no column matches, so the two composers - // below hand a string to the catalog. The empty string is falsy exactly - // where the missing label was, which is what the announcement's guard reads. + // The announcements name the column label, not the descriptor's id. Empty + // rather than absent, so the composers below always hand over a string. const activeLabel = columns.find((column) => column.id === state.sortColumnId)?.label ?? ""; - // The four views the table can show, chosen once here rather than through a - // stack of conditional expressions inside the markup. The order is the - // precedence: a failure outranks a pending load, and both outrank a result. + // The four views the table can show. The order is the precedence: a failure + // outranks a pending load, and both outrank a result. let body: ReactNode; if (errorMessage !== null) { body = ( ); } else if (!datasetReady) { - // The whole view is replaced until the collection has arrived once, the - // first paint before the request even starts included: the empty result - // copy would otherwise claim a search had been made and matched nothing. - // Once the collection has arrived, a refetch keeps the table mounted so it - // does not unmount and flash on every keystroke. + // Gated on datasetReady rather than loading, or the empty-result copy + // would claim a search matched nothing before one was made. body =
{labels.loading}
; } else if (paginatedData.length === 0) { body =
{labels.empty}
; diff --git a/src/components/DataTable/Pagination.tsx b/src/components/DataTable/Pagination.tsx index 0800a63..ad59028 100644 --- a/src/components/DataTable/Pagination.tsx +++ b/src/components/DataTable/Pagination.tsx @@ -9,15 +9,7 @@ import { import { PAGE_SIZE_OPTIONS } from "./tableState"; import styles from "./Pagination.module.scss"; -/** - * Every string the page controls render. - * - * A slice of the table's own labels object rather than a second prop the caller - * assembles, so one object reaches the table and the table hands this part of it - * on. Each of the four action entries is used twice, once as the tooltip and - * once as the accessible name, which is what stops a translation moving one and - * leaving the other behind. - */ +/** Each action entry is read twice, as tooltip and as accessible name. */ export interface PaginationLabels { /** Names the control choosing how many rows a page holds. */ readonly pageSize: string; @@ -44,19 +36,7 @@ interface PaginationProps { readonly labels: PaginationLabels; } -/** - * The page-size control and the page navigation. - * - * It knows the page position and nothing else about the table's view state, so - * a change to how sorting is held cannot reach it. The select's value is parsed - * to a number here, at the only place that sees the event, so the callback - * never receives a string. - * - * Every word it shows arrives in the labels object, the table's own slice of - * which is handed down whole rather than spread or reshaped. Each of the four - * actions reads one entry twice, once as the tooltip and once as the accessible - * name, which is the only arrangement in which the two cannot drift apart. - */ +/** The page-size control and the page navigation, and no other view state. */ export function Pagination({ page, totalPages, @@ -65,15 +45,10 @@ export function Pagination({ onPageSizeChange, labels, }: PaginationProps) { - // Derived rather than a fixed string, so two tables on one page do not label - // each other's select. + // Derived, so two tables on one page do not label each other's select. const pageSizeId = useId(); - /** - * Turns the select's string value into the number the arithmetic divides by. - * The option list is a closed set, so nothing out of range or non-numeric can - * reach the arithmetic through this control. - */ + /** The option list is closed, so the parse cannot yield an unusable size. */ const handlePageSizeChange = (e: React.ChangeEvent) => { onPageSizeChange(Number.parseInt(e.target.value, 10)); }; diff --git a/src/components/DataTable/TableBody.tsx b/src/components/DataTable/TableBody.tsx index 29c587b..09a792f 100644 --- a/src/components/DataTable/TableBody.tsx +++ b/src/components/DataTable/TableBody.tsx @@ -7,15 +7,7 @@ interface TableBodyProps { readonly getRowId: (row: T) => string; } -/** - * The data rows: one row per element, one cell per column, each cell produced - * by that column's own renderer. - * - * The renderer's return value is inserted as a child, which the framework - * escapes, and the default renderer stringifies the value. That is the whole - * defence for a cell slot an author controls, and it is enough only for as long - * as no raw-markup insertion appears anywhere near it. - */ +/** The data rows, each cell produced by its own column's renderer. */ export function TableBody({ rows, columns, diff --git a/src/components/DataTable/TableHead.tsx b/src/components/DataTable/TableHead.tsx index 559f769..6e1b054 100644 --- a/src/components/DataTable/TableHead.tsx +++ b/src/components/DataTable/TableHead.tsx @@ -11,13 +11,7 @@ interface TableHeadProps { readonly onSortChange: (columnId: Id) => void; } -/** - * The header cell's sort state, in the three values the attribute accepts. - * - * Every column carries one: a sortable column that is not the active one - * reports "none" rather than omitting the attribute, so assistive technology - * describes it as sortable-but-unsorted instead of not sortable at all. - */ +/** An inactive column reports "none" rather than omitting the attribute. */ function ariaSort( direction: "asc" | "desc" | null, ): "ascending" | "descending" | "none" { @@ -26,15 +20,7 @@ function ariaSort( return "none"; } -/** - * The header row: one cell per column, each carrying the control that cycles - * the sort and the state the cycle is currently in. - * - * Everything rendered here comes from the descriptor, so this component names - * no field of the row type it orders. The width the descriptor may carry is - * applied as a style object rather than interpolated into an attribute, which - * is what keeps an author-supplied string off the attribute itself. - */ +/** The header row. The width is a style object, not an interpolation. */ export function TableHead({ columns, sortColumnId, diff --git a/src/components/DataTable/column.ts b/src/components/DataTable/column.ts index 1b2538f..abc8055 100644 --- a/src/components/DataTable/column.ts +++ b/src/components/DataTable/column.ts @@ -2,55 +2,22 @@ import type { ReactNode } from "react"; import { compareValues } from "../compareRows"; -/** - * A column, with its value type already fused in and then erased. - * - * The value type is deliberately absent from this interface. Keeping it as a - * second type parameter is the shape every table library reaches for, and it - * does not compile here: under strictFunctionTypes with property syntax, a - * column over a string value is not assignable to a column over an unknown one, - * so a heterogeneous array collapses the moment it is annotated or crosses a - * prop boundary. The method-syntax workaround compiles only because methods are - * bivariant, which is unsound, and it hands the cell renderer a value it cannot - * narrow. Fusing the value type into the two functions at construction and - * dropping it from the element type costs the table the ability to inspect a - * value, which the table never needed, and buys an array of columns over - * different value types that survives being passed around. - */ +/** Its value type is fused into the two functions below, then erased. */ export interface Column { - /** - * Unique within the array the column is rendered in. A repeat collides on - * the cell key React reconciles a row by and on the lookup that resolves the - * sort column. The builder below covers the array one builder produces, which - * is every array in this tree; concatenating two builders' output is not - * checked anywhere. - */ + /** Unique within its array: a repeat collides on React's own cell key. */ readonly id: Id; readonly label: string; - /** - * Declared now and set by nothing. Row virtualization needs a width it can - * measure without reading the DOM, and amending this interface twice is the - * cost that buys. - */ + /** Declared for a future row virtualizer and set by nothing today. */ readonly width?: string | undefined; - /** - * The column carries a number. The table decides what that looks like. - */ + /** The column carries a number. The table decides what that looks like. */ readonly numeric?: boolean | undefined; readonly renderCell: (row: T) => ReactNode; readonly compare: (a: T, b: T, direction: "asc" | "desc") => number; } /** - * What a caller supplies. Everything here is value-level: the accessor has - * already been applied by the time a renderer or a comparator declared here is - * called, which is what lets the value type be inferred once, at the call site, - * with no annotation. - * - * The comparator takes the direction rather than being flipped by its caller. - * Blanks sort last in both directions, which is a rule a direction-free - * comparator cannot express: negating it puts every blank first on descending, - * and on real data that is a first page of empty cells. + * What a caller supplies. The comparator takes the direction rather than being + * flipped by its caller, because blanks sort last in both. * * ponytail: a caller-supplied comparator keeps the three parameters it has * always had and never receives the collator, so a caller that wants to collate @@ -68,43 +35,14 @@ export interface ColumnOptions { } /** - * Builds columns for one row type. - * - * Curried because TypeScript infers all of a call's type arguments or none of - * them. The row type is the one thing a caller knows and the compiler cannot - * guess, so it is supplied here; the column id and the value type are then - * inferred per call, which is the whole point. - * - * The id is constrained to string and every call site passes a string literal, - * so it infers as that literal, not as string. An array of these carries the - * literal union of its ids with no assertion written anywhere, and renaming a - * column is a compile error at every use site rather than a silent widening. - * - * The const modifier on the two id parameters below is redundant, since a - * scalar string parameter already infers a literal from a literal argument. - * It changes inference only for a parameter that takes an array or object, - * which is not this signature. - * - * The collator is supplied here rather than reaching the comparison some other - * way, and it is what makes the column array the carrier of the reader's - * locale. The array is rebuilt per locale anyway, for its labels and for the - * cells that format a number, so fusing the collator in at construction costs - * one parameter and leaves the sort module, its hook and the table's own prop - * surface entirely untouched. A collator is a platform value, so taking one - * here leaves this layer's dependency set exactly as it was. - * - * One builder builds one table's columns, and throws on an id it has already - * issued. A second, unrelated table takes a second builder. + * Builds columns for one row type. Curried because TypeScript infers all of a + * call's type arguments or none, so an array carries its literal id union with + * no assertion. One builder per table: it throws on a repeated id. */ export function columns(collator: Intl.Collator) { const issued = new Set(); - /** - * The one place where the accessor is fused into the renderer and the - * comparator, so the two public methods below differ only in how they read a - * value. Supplying neither leaves the value stringified for display, blank - * if it is nullish, and ordered by the shared comparator. - */ + /** The one place the accessor is fused into the renderer and comparator. */ function build( id: Id, read: (row: T) => V, @@ -125,9 +63,8 @@ export function columns(collator: Intl.Collator) { renderCell: renderCell ? (row) => renderCell(read(row), row) : (row) => { - // Nullish paints an empty cell rather than the word "null". NaN - // is not covered: the comparator calls it blank, but a cell reading - // "NaN" is worth seeing. + // Nullish paints an empty cell rather than the word "null". NaN is + // not covered: the comparator calls it blank, but it is worth seeing. const value = read(row); return value == null ? "" : String(value); }, @@ -139,11 +76,7 @@ export function columns(collator: Intl.Collator) { } return { - /** - * A column whose id is one of the row type's own string keys, and whose - * value is that field. Constrained to the string keys because a number or - * symbol key cannot be a column id. - */ + /** A column over one of the row type's own string keys. */ key>( id: Id, options: ColumnOptions, @@ -151,10 +84,7 @@ export function columns(collator: Intl.Collator) { return build(id, (row) => row[id], options); }, - /** - * A column whose value is computed rather than read, so its id is free of - * the row type and its value type is inferred from the read function. - */ + /** A computed column: its id is free of the row type, its value inferred. */ accessor( id: Id, read: (row: T) => V, diff --git a/src/components/DataTable/sortRows.ts b/src/components/DataTable/sortRows.ts index 5418ade..be5ca35 100644 --- a/src/components/DataTable/sortRows.ts +++ b/src/components/DataTable/sortRows.ts @@ -1,20 +1,8 @@ import type { Column } from "./column"; /** - * Orders rows by one column, or hands them back untouched when nothing is - * sorted. - * - * The identity tiebreak lives here rather than inside a column's comparator - * because the function that produces a row's identity is a table-level prop: - * a column is defined without knowing what identifies a row, and this is the - * first place both are in scope at once. It runs after the column comparator - * has had its say, and it is never flipped by the direction, which keeps it one - * rule a reader can state in a line rather than a rule with an exception. It is - * also what makes sorting the same set twice produce the same order, rather - * than whichever order the rows happened to arrive in. - * - * What that tiebreak decides, and what the caller owes it, is on - * compareIdentities below. + * Orders rows by one column. Row identity is a table-level prop, so the + * tiebreak lives here rather than inside a column's comparator. */ export function sortRows( rows: readonly T[], @@ -24,8 +12,8 @@ export function sortRows( ): readonly T[] { if (!column || !direction) return rows; - // The resolved collection is module-cached and shared by every reader, so - // it is treated as immutable and the sort runs over a copy. + // The resolved collection is module-cached and shared, so it is treated as + // immutable and the sort runs over a copy. return [...rows].sort((a, b) => { const comparison = column.compare(a, b, direction); if (comparison !== 0) return comparison; @@ -35,25 +23,8 @@ export function sortRows( } /** - * Orders two row identities, ascending and never flipped by the direction. - * - * Identities order as text, so an identity of "100" comes before "99". That is - * a real ordering a reader can see, not a detail below the surface: it decides - * every pair whose column values compare equal, and a column with many equal - * values leaves most of the table to this rule. Supplying identities that sort - * as text the way their subjects sort is therefore the caller's job, and the - * caller is the only one who knows what a row's identity means. - * - * Deliberately not collated, and that is the one thing to keep true here. The - * column comparator above orders text by the reader's resolved locale, which is - * what a reader expects of the values they can see. An identity is not a value - * they can see: it decides every pair the column left tied, so collating it - * would let the same data come out in two orders for two readers with every - * visible value equal. Ordering identities as plain text is what keeps that one - * rule the same for everyone. - * - * Exported so a caller reproducing the table's order compares identities the - * way the table does, rather than restating the rule and drifting from it. + * Ascending, never flipped, and never collated: an identity is not a value the + * reader sees, so collating it would give two readers two orders. */ export function compareIdentities(aId: string, bId: string): number { if (aId === bId) return 0; diff --git a/src/components/DataTable/tableState.ts b/src/components/DataTable/tableState.ts index 51656be..c9f54fa 100644 --- a/src/components/DataTable/tableState.ts +++ b/src/components/DataTable/tableState.ts @@ -1,21 +1,11 @@ -/** - * Everything the table remembers between renders, in one object. - * - * It is one object rather than five values because it is also what gets read - * whole: a serialiser writes all of it at once and a restored address writes - * all of it back at once, and five separate writes are five chances to tear. - */ +/** Everything the table remembers. Five separate writes would be five tears. */ export interface TableState { readonly sortColumnId: Id | null; readonly sortDirection: "asc" | "desc" | null; readonly page: number; readonly pageSize: number; readonly query: string; - /** - * A cleared sort and a sort that has never been applied render the same - * state, so the announcement needs to know which of the two it is looking at: - * the first render has to stay silent, the third press on a column does not. - */ + /** A cleared sort and a sort never applied otherwise look the same. */ readonly hasSorted: boolean; } @@ -26,14 +16,8 @@ export type TableAction = | { readonly type: "query"; readonly query: string }; /** - * Where a table starts, with three readers: the state below it, whatever - * serialises the state (a value equal to one of these is the value to leave - * out), and whatever parses it back (a parameter that fails validation falls - * back to the value here). One owner is what keeps those three agreeing. - * - * Typed over no column id at all, which makes it assignable to a table state - * over any id union, since the only place an id appears is a field that may - * also be null. + * Where a table starts: what the serializer omits and the parser falls back to. + * Typed over no column id, so it is assignable to a state over any id union. */ export const DEFAULT_TABLE_STATE: TableState = { sortColumnId: null, @@ -44,26 +28,10 @@ export const DEFAULT_TABLE_STATE: TableState = { hasSorted: false, }; -/** - * The page sizes the table offers, in the order it offers them. - * - * One owner because this list is two things at once: the surface a reader picks - * from, and the rule that decides whether a size arriving from outside the - * application is one the table can represent. Two independent copies of a list - * with no mechanism keeping them in step is a failure mode this project has - * already filed once, and it costs one line here to avoid. - * - * Not narrowed with a const assertion: the membership test reads an arbitrary - * number, so a union of the four literals would be a type the caller cannot - * hand a value to. - */ +/** The page sizes offered, and the rule validating a size from outside. */ export const PAGE_SIZE_OPTIONS: readonly number[] = [10, 25, 50, 100]; -/** - * The fields an action changes, before the page reset is applied over them. - * Split out so the reset below can be written once for all three actions that - * trigger it rather than repeated inside each of their branches. - */ +/** The fields an action changes, before the shared page reset lands on top. */ function changedBy( state: TableState, action: Exclude, { type: "page" }>, @@ -75,10 +43,8 @@ function changedBy( return { query: action.query }; case "sort": { const { columnId } = action; - // Sorting cycles rather than toggling: a new column starts ascending, the - // active one goes ascending to descending to unsorted, and a cleared - // column starts over. Every branch reports that a sort happened, - // including the one that clears it. + // Sorting cycles rather than toggling: ascending, descending, unsorted, + // then over. Every branch reports a sort, the one that clears included. if (state.sortColumnId !== columnId) { return { sortColumnId: columnId, @@ -97,42 +63,27 @@ function changedBy( } } -/** - * The only thing that moves the table from one state to the next. - * - * It is pure and knows nothing about React, so the rules below can be read and - * tested without rendering anything. - */ +/** The only thing that moves the table from one state to the next. */ export function applyTableAction( state: TableState, action: TableAction, ): TableState { if (action.type === "page") { - // Taken exactly as given, never corrected against the pages that happen to - // exist. Correcting here would store a position the user did not choose, so - // a result set that widens again could not restore them, and a position - // arriving before its rows do would be corrected against no rows at all. - // The correction belongs where the rows are counted, and it belongs to the - // read rather than to the write. + // Taken exactly as given. Correcting here would store a position the reader + // did not choose, and a position arriving before its rows would be + // corrected against no rows at all. The clamp belongs to the read. return { ...state, page: action.page }; } - // A term that settles back where it started leaves the same rows in the same - // order, so the position chosen against them still means what it meant. This - // is the one action a control can emit at its current value: a sort press and - // a size selection are always a real change, while the debounce commits any - // sequence of keystrokes that pauses, including one that undoes itself. The - // same state object rather than an equal one, so the render and the address - // write behind it never run for a change that did not happen. + // The one action a control can emit at its current value, because the + // debounce commits any pause, including a sequence that undoes itself. The + // same object rather than an equal one, so no render and no address write. if (action.type === "query" && action.query === state.query) { return state; } - // Sorting, resizing the page, and searching each replace the set of rows the - // position was chosen against, so all three return to the first page. Fused - // into one return so that reset has exactly one site in the codebase: whoever - // adds a fifth action has to decide about it rather than forget it. The guard - // above is a guard against the action, not a condition on the reset: once an - // action reaches this line it resets, whatever it carries. + // Sorting, resizing and searching each replace the rows the position was + // chosen against, so all three return to the first page. One site, so whoever + // adds a fifth action has to decide about the reset rather than forget it. return { ...state, ...changedBy(state, action), page: 1 }; } diff --git a/src/components/DataTable/tableStateUrl.ts b/src/components/DataTable/tableStateUrl.ts index 9fcfe51..0d88989 100644 --- a/src/components/DataTable/tableStateUrl.ts +++ b/src/components/DataTable/tableStateUrl.ts @@ -4,33 +4,12 @@ import { type TableState, } from "./tableState"; -/** - * What a descending sort is written with, ahead of the column id. - * - * One key rather than a column and a direction, and not to shorten the link: - * the two fields are coupled in the state, where the direction is null exactly - * when the column is, so a single token makes the invalid pair unrepresentable - * rather than merely rejected and the parser needs no cross-field rule at all. - * - * A hyphen rather than a colon-separated form, which has the same validation - * property but is percent-encoded in some of the paths a link travels, so the - * pasted address reads worse for no gain. - */ +/** One key rather than two, so the invalid pair is unrepresentable. */ const SORT_DESCENDING_PREFIX = "-"; /** - * One parameter this schema owns: the key it answers to, how a raw value is - * read back into view state, and how the state's value for it is written out. - * - * The reading half returns a partial rather than a single value, so a key that - * carries two coupled state fields needs no second shape and no cross-field - * rule after the fact. The writing half returns null for a value equal to its - * default, so the rule that keeps defaults out of the address is applied once - * over the table below rather than restated inside every entry. - * - * Both halves are generic over the column ids per call rather than over the - * interface, which is what lets the schema be one module-scope array shared by - * every table instead of one array per row type. + * One owned parameter. Parsing returns a partial so one key can carry two + * coupled fields; serializing returns null for a defaulted value. */ interface UrlParamEntry { readonly key: string; @@ -43,30 +22,14 @@ interface UrlParamEntry { ) => string | null; } -/** - * The parameters this application owns, in the order it writes them. - * - * The array's order is the canonical order, so there is no second list of key - * names to keep in step with it, and adding a parameter is one entry rather - * than an edit to a parser, a serializer, and an order. - */ +/** The parameters this application owns, in the canonical write order. */ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ { key: "q", - // Taken exactly as it arrives, with nothing to validate. A term is a free - // string by nature: it reaches a controlled input's value and a substring - // match over the collection, never a lookup and never markup, so there is - // no shape it could fail to have. Every rule a term does have, the spelling - // of a space and the encoding of the punctuation the query string uses for - // itself, belongs to the query serializer below rather than here. + // A term reaches a controlled value and a substring match, never a lookup. parse: (raw) => ({ query: raw }), - // Trimmed on the way out, because the search itself trims: a term carrying - // edge whitespace selects the same rows as one without, so writing it - // verbatim would give one view two addresses that never converge, and a - // term that is nothing but whitespace would write a key for a view - // identical to the default. Trimmed here rather than in the state, so what - // the reader is typing stays painted in the box exactly as they typed it - // while the address stays canonical. + // Trimmed on the way out because the search trims, so one view cannot have + // two addresses. Not trimmed in the state, so the box shows what was typed. serialize: (state) => { const term = state.query.trim(); return term === DEFAULT_TABLE_STATE.query ? null : term; @@ -74,19 +37,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "sort", - // The token is checked by locating it among the ids the caller supplied, - // comparing values rather than indexing anything. Located with find rather - // than tested and then asserted, so the result arrives already typed as one - // of those ids and nothing here has to claim a type for a string that came - // out of the address. - // + // Located with find, so the result arrives already typed as a caller's id. // The whole token is tried before the prefix is stripped, because an id may - // itself begin with the prefix and nothing constrains it not to. Stripping - // first makes such an id unreachable ascending: it writes a token that - // reads back as a different id descending, so the sort is silently dropped - // rather than restored. Trying the exact match first leaves exactly one - // ambiguity, between an id and the id the prefix would produce from it, and - // resolves it toward the one that exists as written. + // begin with it: stripping first leaves such an id unreachable ascending. parse: (raw, validColumnIds) => { const ascending = validColumnIds.find((candidate) => candidate === raw); if (ascending !== undefined) { @@ -111,19 +64,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "page", - // Coerced whole rather than with the radix parser, which truncates in two - // opposite directions: it reads exponent notation as a single digit and a - // number carrying trailing text as its numeric prefix. The page-size select - // can use it safely because its input is a fixed option list; the address - // is not a fixed list. - // - // Any positive integer is taken, with no upper bound. The clamp that turns - // a position into rows is what bounds it, and an out-of-range value has to - // survive in the address rather than be corrected back into it. That also - // means hexadecimal notation is accepted as the number it denotes, which is - // a choice rather than an oversight: it is a positive integer, the clamp - // bounds it, and a format rule here would be a rule with no failure it - // prevents. + // Coerced whole rather than with the radix parser, which reads exponent + // notation as a single digit. Any positive integer, with no upper bound: + // the read-side clamp is what bounds it. parse: (raw) => { const page = Number(raw); return Number.isInteger(page) && page > 0 ? { page } : undefined; @@ -133,12 +76,8 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "size", - // Accepted only when it is a size the table offers, because the table's own - // select cannot represent one that is not among its options: accepting an - // arbitrary size would render a control whose value is not in its own list. - // Membership in that list already implies a whole number, so there is no - // second predicate to write. Coerced whole for the same reason the position - // above is. + // Only a size the table offers, because the select cannot represent one + // that is not among its options. Membership already implies a whole number. parse: (raw) => { const pageSize = Number(raw); return PAGE_SIZE_OPTIONS.includes(pageSize) ? { pageSize } : undefined; @@ -151,17 +90,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ ]; /** - * Reads whatever of the view state a query string carries. - * - * Total by construction: a value that fails validation is left out rather than - * replaced, and the caller spreads the result over the default state, so an - * omitted field is the default and no parameter needs a fallback of its own. - * - * The valid column ids arrive as an argument rather than being imported, which - * is what keeps this module ignorant of what its rows are, exactly as the table - * receives its columns. Nothing read out of the query is ever used as an object - * key and nothing is deep merged, so a parameter named after a prototype member - * is structurally harmless rather than a case in a validator. + * Reads whatever of the view state a query string carries. Total by + * construction: a value failing validation is left out, so no parameter needs a + * fallback arm. The column ids arrive as an argument. */ export function parseTableState( search: string, @@ -171,9 +102,8 @@ export function parseTableState( const restored: Partial> = {}; for (const entry of PARAM_SCHEMA) { - // The first occurrence of a repeated key, which is the whole rule for one: - // the extras are dropped by the write that follows rather than by a rule of - // their own. + // The first occurrence of a repeated key; the extras are dropped by the + // write that follows rather than by a rule of their own. const raw = params.get(entry.key); if (raw === null) continue; @@ -187,15 +117,9 @@ export function parseTableState( } /** - * Writes the view state back out, preserving every parameter it does not own. - * - * Owned keys are written first in the schema's order and anything else follows - * in the order it arrived, so two equivalent views produce the same string - * while a tracking tag someone else put in the link survives the write. - * - * The result is shaped the way the address bar's own query is shaped: empty - * when there is nothing to say, and otherwise a question mark followed by the - * parameters. That is what lets the caller's write guard be a bare comparison. + * Writes the view state back out, preserving every parameter it does not own + * and shaped like the address bar's own query, so the write guard is a bare + * comparison. */ export function serializeTableState( state: TableState, @@ -212,8 +136,8 @@ export function serializeTableState( } for (const [key, value] of incoming) { - // Ownership is decided by comparing against the schema's own key strings, - // never by looking the incoming key up in an object. + // Ownership is decided by comparing key strings, never by looking the + // incoming key up in an object. if (!PARAM_SCHEMA.some((entry) => entry.key === key)) { next.append(key, value); } @@ -223,15 +147,7 @@ export function serializeTableState( return query === "" ? "" : `?${query}`; } -/** - * Reads only the search term a query string carries. - * - * The container behind the table needs the term on its very first render and - * owns none of the columns, so it reads through this rather than passing an - * empty column-id list to say it does not care about the sort. One schema and - * two entry points: the rules still live in exactly one place, and a reader of - * the call site can see what is being asked for. - */ +/** Reads only the term, for the container that owns none of the columns. */ export function parseSearchTerm(search: string): string { return parseTableState(search, []).query ?? DEFAULT_TABLE_STATE.query; } diff --git a/src/components/SearchInput.tsx b/src/components/SearchInput.tsx index c0a7916..84a58ef 100644 --- a/src/components/SearchInput.tsx +++ b/src/components/SearchInput.tsx @@ -2,12 +2,7 @@ import { FiSearch } from "react-icons/fi"; import styles from "./SearchInput.module.scss"; -/** - * The two strings this control shows. - * - * One object rather than two loose props, so the pair moves together when the - * language does and a caller cannot supply half of it. - */ +/** The two strings this control shows, as one object so they move together. */ export interface SearchInputLabels { /** The accessible name: what the control does, not what it searches. */ readonly name: string; @@ -22,12 +17,9 @@ interface SearchInputProps { } /** - * The search box above the table. - * - * The term is reported upward rather than held here, so the control stays a - * pure function of the value its owner already has. The event-to-term - * conversion happens here, at the only place that knows an input event exists, - * which keeps the callback signature free of the DOM. + * The search box above the table. The term is reported upward rather than held + * here, and the event-to-term conversion happens at the only place that knows + * an input event exists, which keeps the callback signature free of the DOM. * * a11y: the accessible name describes what the control does rather than what it * searches, which is why it is one word and not the name of a collection. That @@ -38,10 +30,7 @@ interface SearchInputProps { * the caller knows. */ export function SearchInput({ value, onChange, labels }: SearchInputProps) { - /** - * Reports every keystroke upward. Debouncing belongs to whoever owns the - * request, not to the control. - */ + /** Reports every keystroke upward; debouncing belongs to the request owner. */ const handleSearchChange = (e: React.ChangeEvent) => { onChange(e.target.value); }; diff --git a/src/components/compareRows.ts b/src/components/compareRows.ts index 2aaddb4..58fa4ab 100644 --- a/src/components/compareRows.ts +++ b/src/components/compareRows.ts @@ -1,47 +1,21 @@ -/** - * A value carrying nothing to order by. A zero is deliberately not blank; the - * dataset records hundreds of cities with no population as 0, and those rows - * belong at the small end of a population sort rather than at the far end with - * the empty strings. - * - * NaN is blank. It is a number by typeof, so without this it reaches the - * subtraction and returns NaN, which is neither negative, positive, nor zero: - * the direction flip leaves it NaN, the row-id tiebreak never runs, and the - * order of a set holding one comes out different for different arrival orders. - */ +/** Nothing to order by. Zero is not blank (0 population sorts small); NaN is. */ function isBlank(value: unknown): boolean { return ( value === "" || value === null || value === undefined || Number.isNaN(value) ); } -/** - * Where a value's type sits in the ordering. Grouping by type before comparing - * within it is what keeps the comparison transitive once the rows stop being - * uniformly typed: numbers order among themselves, everything else collates as - * text, and the two groups never have to be compared by a rule that disagrees - * with the rule used inside them. - */ +/** Grouping by type keeps the comparison transitive over mixed rows. */ const TYPE_RANK = { number: 0, string: 1, other: 2 } as const; -/** - * Places one value in the ordering above, dispatching on its runtime type - * because the rows reach here already widened and a declared type is not - * available to dispatch on. - */ +/** Places a value in the ordering above. The rows reach here already widened. */ function rank(value: unknown): number { if (typeof value === "number") return TYPE_RANK.number; if (typeof value === "string") return TYPE_RANK.string; return TYPE_RANK.other; } -/** - * Orders two values that are both present, before any direction is applied. - * - * Type is the primary key, so which rule decides a pair is settled by the pair's - * types rather than by the values: without that, a numeric pair and a - * stringified pair can each be decided by a different rule and produce a cycle. - */ +/** Orders two present values. Type is the primary key, so no pair cycles. */ function compareRanked( aValue: unknown, bValue: unknown, @@ -52,11 +26,8 @@ function compareRanked( if (aRank !== bRank) return aRank - bRank; if (aRank === TYPE_RANK.number) { - // Compared rather than subtracted. Two infinities of the same sign subtract - // to NaN, which would skip the identity tiebreak the sort module applies - // after this returns and leave the order up to the sort, and a subtraction - // of two large magnitudes reports a difference where a direction is all - // that is wanted. + // Compared rather than subtracted: two infinities of the same sign subtract + // to NaN, which would skip the sort module's identity tiebreak. const aNumber = aValue as number; const bNumber = bValue as number; if (aNumber === bNumber) return 0; @@ -67,40 +38,9 @@ function compareRanked( } /** - * Orders two already-widened values. - * - * The three rules below compose in an order that is load-bearing, so read them - * as a sequence rather than as a set. - * - * The blank test runs first, ahead of the direction flip, which is what puts - * blanks last whichever way the column is sorted. Capital is empty on roughly - * two thirds of the rows, so the other rule leads the first ascending page with - * a screen of empty cells and reads as a broken table. - * - * The typed comparison runs second, and its result is the only thing the - * direction flip touches. Type is the primary key there, so which rule decides - * a pair is settled by the column rather than by the pair: without that, a - * numeric pair and a stringified pair can each be decided by a different rule - * and produce a cycle, which is an ordering the sort is free to resolve however - * it likes. The parse boundary guarantees each field's type today, so the - * dataset cannot reach the mixed arms; they are here for the point at which the - * table becomes generic over its row type and that guarantee stops covering the - * input. - * - * The row-identity tiebreak that used to run last is no longer here. It ran on - * a field of the row, and this function no longer sees a row: identity is a - * table-level prop, not a fact a column can know about itself. It now runs in - * the sort module, after this function returns and never flipped, so the rule - * itself is unchanged. - * - * The collator arrives as an argument, and it has no default. This module held - * one for its own lifetime while the ordering was whatever the machine running - * the code happened to prefer; text now collates by the reader's resolved - * locale, so the instance is a function of that locale and cannot be a constant - * here. A default would be the locale-less collator this parameter exists to - * remove, and it would hide a call site from the source guard that keeps every - * platform locale construction in one module. This layer still imports nothing - * from the locale layer: a collator handed in is a value, not a dependency. + * Orders two already-widened values. Blanks are tested ahead of the direction + * flip so they stay last either way, and the collator is a parameter because + * this layer may not import the locale layer. */ export function compareValues( aValue: unknown, @@ -112,14 +52,11 @@ export function compareValues( const bBlank = isBlank(bValue); if (aBlank !== bBlank) return aBlank ? 1 : -1; - // Both blank compares equal: the arm above has already answered every pair - // where only one of them is. + // Both blank compares equal; the arm above answered every one-sided pair. const comparison = aBlank ? 0 : compareRanked(aValue, bValue, collator); - // Returned ahead of the flip so an equal pair comes back as a positive zero - // in both directions. Negating zero gives negative zero, which every ordering - // rule downstream reads as a tie but which an equality check does not: the - // pair would compare equal ascending and not equal descending. + // Returned ahead of the flip so an equal pair is a positive zero either way. + // Negating zero gives -0, which an equality check downstream reads as unequal. if (comparison === 0) return 0; return direction === "desc" ? -comparison : comparison; } diff --git a/src/components/paginate.ts b/src/components/paginate.ts index cbe8e6f..40d57c4 100644 --- a/src/components/paginate.ts +++ b/src/components/paginate.ts @@ -1,7 +1,4 @@ -/** - * One page read out of a collection: the rows on that page, how many pages the - * collection has, and which page was actually read. - */ +/** One page read out of a collection: rows, page count, page actually read. */ export interface PaginateResult { readonly paginatedData: readonly T[]; readonly totalPages: number; @@ -9,27 +6,18 @@ export interface PaginateResult { } /** - * Slices one page out of a collection. - * - * The page position is an argument rather than something this function owns, - * and the correction it applies below never leaves this function, which is what - * lets a position arrive from anywhere: a click, a restored URL, or a render - * that happens before the rows do. + * Slices one page out of a collection. The position is an argument and the + * clamp never leaves this function, so it can arrive from anywhere. */ export function paginate( rows: readonly T[], page: number, pageSize: number, ): PaginateResult { - // Floored at one so an empty result set still counts as a single page. - // Zero would be a page count nothing can be on, and callers outside the - // navigation's own visibility guard have no protection from it. + // Floored at one: zero would be a page count nothing can be on. const totalPages = Math.max(1, Math.ceil(rows.length / pageSize)); - // Clamped for reading only. The position held in state is deliberately - // left alone, so a result set that widens again restores the user where - // they were rather than stranding them on whatever the narrowed set - // allowed, and so a position arriving from outside survives a fetch that - // has not resolved yet. + // Clamped for reading only. The position in state is left alone, so a result + // set that widens again restores the reader where they were. const effectivePage = Math.min(Math.max(page, 1), totalPages); const startIndex = (effectivePage - 1) * pageSize; return { diff --git a/src/hooks/useDebouncedCallback.ts b/src/hooks/useDebouncedCallback.ts index cd48ece..d3abef1 100644 --- a/src/hooks/useDebouncedCallback.ts +++ b/src/hooks/useDebouncedCallback.ts @@ -1,43 +1,14 @@ import { useCallback, useEffect, useMemo, useRef } from "react"; -/** - * A debounced call and the means to drop one that is already pending. - * - * Two named members rather than a cancel hung on the scheduler: attaching one - * means mutating a memoized function during render, which is the shape this - * project's lint gate rejects. Both members are stable, so a caller that - * destructures the pair can put either in a dependency array; the wrapper is - * memoized too, for a caller that holds it whole. - */ +/** A debounced call, plus the means to drop one that is already pending. */ export interface DebouncedCallback { readonly schedule: (...args: A) => void; readonly cancel: () => void; } /** - * Debounces a call rather than a value: the scheduler it returns schedules the - * callback and reschedules it on every further call, so a burst of calls - * settles into one invocation carrying the arguments of the last. - * - * Debouncing the call is what keeps this usable from an event handler. A - * debounced value has to be turned into a state write or a callback somewhere, - * and the only place left for that is an effect, which is the shape this - * project's lint gate now rejects. Scheduling inside the handler makes the - * commit an ordinary event-driven write instead. - * - * The pending handle lives in a ref and is cleared on unmount, so a call still - * in flight when the component tears down never lands. The cancel covers the - * case that is not a teardown: a caller that has just replaced the state a - * pending call was made against needs to drop that call, and without a cancel - * its only alternative is to let the stale value land on top of the new one. - * - * The callback is read out of a ref when the timer fires rather than closed - * over when the call was scheduled, so a pending call always runs the current - * implementation instead of the one that happened to be current a moment - * earlier. That is also what lets the scheduler be memoized over the delay - * alone: it holds one identity for the life of the component whatever the - * caller does with its own, so an inline arrow is as safe here as a memoized - * callback rather than being a stale-closure trap the types cannot catch. + * Debounces a call rather than a value, and reads the callback from a ref when + * the timer fires, which is what makes an inline arrow safe to pass. */ export function useDebouncedCallback( callback: (...args: A) => void, @@ -63,10 +34,8 @@ export function useDebouncedCallback( (...args: A) => { clearTimeout(pending.current); pending.current = setTimeout(() => { - // Dropped as it fires, for the same reason the cancel below drops it: - // a handle the platform has already retired must not be clearable a - // second time, and a call that has landed retires its own handle just - // as surely as a cancel does. + // Dropped as it fires: a handle the platform has already retired must + // not be clearable a second time. pending.current = undefined; latest.current(...args); }, delay); @@ -74,9 +43,8 @@ export function useDebouncedCallback( [delay], ); - // The handle is dropped as well as cleared, so a cancel followed by another - // cancel clears nothing rather than an identifier the platform has since - // handed to someone else's timer. + // The handle is dropped as well as cleared, so a second cancel clears nothing + // rather than an identifier the platform has since reissued. const cancel = useCallback(() => { clearTimeout(pending.current); pending.current = undefined; diff --git a/src/hooks/useLocale.ts b/src/hooks/useLocale.ts index ce1b40d..b1ae607 100644 --- a/src/hooks/useLocale.ts +++ b/src/hooks/useLocale.ts @@ -9,38 +9,24 @@ import { } from "../i18n/localeStore"; /** - * Owns the reader's locale for one component: the choice, the catalog its - * strings come from, the tag the platform formatters take, and the two - * attributes the document element carries. - * - * Many instances by construction, which is the opposite of the theme hook - * beside it and worth stating rather than inheriting. That hook holds its choice - * per caller, so two callers hold two choices and their two effects race on one - * document element. This one holds no state at all: the choice lives in a single - * module-scope store, so every subscriber resolves the identical value and the - * duplicate writes below agree by construction rather than by there being only - * one of them. - * - * The language attribute takes the resolved tag rather than the catalog id. The - * tag is the field that is a well formed language tag for every catalog, the - * pseudo-locale's strings really are English, and it keeps the attribute naming - * the same locale the platform formatters are given. + * Owns the reader's locale for one component. Many instances by construction: + * it holds no state, so every subscriber resolves the identical value. */ export function useLocale() { const locale = useSyncExternalStore(subscribeLocale, getLocaleSnapshot); const choice = useSyncExternalStore(subscribeLocale, getChoiceSnapshot); useEffect(() => { - // Both, because the inline script sets both before first paint and the one - // left unmaintained goes stale the first time the reader chooses. + // Both, because the inline script sets both before first paint and either + // one left unmaintained goes stale the first time the reader chooses. document.documentElement.lang = locale.tag; document.documentElement.dir = locale.dir; }, [locale]); return { choice, - // The store's own setter, which already holds one identity for the life of - // the document, so nothing here has to memoize it back into one. + // The store's own setter, already one identity for the life of the + // document, so nothing here has to memoize it back into one. setChoice: setLocaleChoice, catalog: CATALOGS[locale.catalog], tag: locale.tag, diff --git a/src/hooks/usePaginatedRows.ts b/src/hooks/usePaginatedRows.ts index 4d9c28a..a05b41d 100644 --- a/src/hooks/usePaginatedRows.ts +++ b/src/hooks/usePaginatedRows.ts @@ -2,14 +2,7 @@ import { useMemo } from "react"; import { paginate, type PaginateResult } from "../components/paginate"; -/** - * Memoizes one page of rows. - * - * The page position it reports back has been clamped to the number of pages - * that exist, which is a value to render and never one to write back into - * state: storing it would strand the user on whatever a narrowed result set - * allowed rather than restoring them when it widens again. - */ +/** Memoizes one page. The clamped position is to render, never to store. */ export function usePaginatedRows( rows: readonly T[], page: number, diff --git a/src/hooks/useSortedRows.ts b/src/hooks/useSortedRows.ts index 293f9f1..883b5a9 100644 --- a/src/hooks/useSortedRows.ts +++ b/src/hooks/useSortedRows.ts @@ -3,15 +3,7 @@ import { useMemo } from "react"; import type { Column } from "../components/DataTable/column"; import { sortRows } from "../components/DataTable/sortRows"; -/** - * Memoizes the sorted rows, resolving the active column id to its descriptor - * inside the memo so the caller passes an id rather than a descriptor it would - * otherwise have to look up and keep stable itself. - * - * Every argument is a dependency, so a caller that rebuilds the column array or - * the identity function on each render defeats the memo and re-sorts the whole - * collection on every keystroke. Both belong at module scope. - */ +/** Memoizes the sorted rows. A column array rebuilt each render re-sorts. */ export function useSortedRows( rows: readonly T[], columns: readonly Column[], diff --git a/src/hooks/useTheme.ts b/src/hooks/useTheme.ts index e2fdf98..de4fbe0 100644 --- a/src/hooks/useTheme.ts +++ b/src/hooks/useTheme.ts @@ -9,21 +9,8 @@ import { import type { ThemeChoice } from "../theme/resolveTheme"; /** - * Reads the stored choice. Anything that is not exactly one of the two explicit - * words is absent, and so is a store that cannot be read at all: a stale entry - * from an older build and a hostile one are the same case, and both render the - * default rather than an undefined theme. - * - * Membership is tested against the declared vocabulary rather than against two - * words written out again here, so the accepted set has one definition. The - * default word is excluded on its own line: the key holding it is already - * treated as absent, because the default has exactly one representation and it - * is the key not being there. - * - * The property access is what throws when site data is blocked, so neither a - * typeof guard nor optional chaining substitutes for the catch. Unguarded, the - * throw happens inside a state initializer, which unmounts the whole tree over a - * display preference. + * Reads the stored choice; anything but the two explicit words is absent. The + * property access is what throws when site data is blocked, hence the catch. */ function readStoredChoice(): ThemeChoice { try { @@ -42,20 +29,12 @@ function readStoredChoice(): ThemeChoice { return "system"; } -/** - * Whether this environment can answer a media query at all. Asked in one place - * rather than two: the reader below and the subscription below that have to - * agree, or the hook subscribes to something it will not read. - */ +/** Asked once, or the reader and the subscription below could disagree. */ function supportsMediaQueries(): boolean { return typeof window.matchMedia === "function"; } -/** - * The operating system's current preference. An environment with no media query - * support prefers light, which is the same answer the resolver gives for a - * missing choice, so nothing downstream has a third case to handle. - */ +/** The system preference. No media query support prefers light. */ function readPrefersDark(): boolean { if (!supportsMediaQueries()) { return false; @@ -65,16 +44,8 @@ function readPrefersDark(): boolean { } /** - * Subscribes to the operating system's preference, returning the unsubscribe. - * - * Paired with the reader above through useSyncExternalStore rather than through - * an effect that seeds state and then re-reads it. The preference can move - * between the render that would seed it and the commit that would subscribe, - * and StrictMode's mount, unmount, remount opens that window a second time; the - * hook closes both by reading the store itself after subscribing. - * - * An environment with no media query support subscribes to nothing and keeps - * the reader's answer, so the two cannot disagree about whether a query exists. + * Subscribes to the system preference. Through useSyncExternalStore rather than + * an effect, which would leave a window for the preference to move. */ function subscribePrefersDark(onStoreChange: () => void): () => void { if (!supportsMediaQueries()) { @@ -90,21 +61,16 @@ function subscribePrefersDark(onStoreChange: () => void): () => void { } /** - * Owns the theme: the stored choice, the write-through when it changes, the - * concrete theme stamped on the document element, and the two subscriptions that - * keep it current, one to the operating system and one to the other tabs. + * Owns the theme: the stored choice, the write-through, the theme stamped on + * the document element, and the subscriptions to the system and the other tabs. * - * Single instance by construction. The choice is per caller, the document - * element the effect stamps it onto is not, so a second caller gets its own - * choice and the two effects race on every render, leaving the loser showing a - * control that disagrees with the painted page. Lift this behind a provider - * mounted once before the second caller exists. + * Single instance by construction. The choice is per caller and the document + * element is not, so two callers hold two choices and their two effects race on + * one element. Lift this behind a provider before adding a second caller. * - * The rule that turns a choice plus a preference into a theme is written a - * second time, as a literal, inside the blocking inline script in index.html. - * That script runs before any module loads, so it cannot import this. A change - * to either one needs the same change to the other, and the parity guard in - * src/toolchain.test.ts is what fails when they stop agreeing. + * The choice-plus-preference rule is written a second time as a literal in the + * blocking inline script in index.html, which cannot import a module. Change + * both together; the parity guard in src/toolchain.test.ts holds them. */ export function useTheme() { const [choice, setChoiceState] = useState(readStoredChoice); @@ -115,10 +81,8 @@ export function useTheme() { useEffect(() => { const handleStorageChange = (event: StorageEvent) => { - // This key only, and only a write from another document: the event does - // not fire in the tab that made it, which is why the setter below does - // not have to guard against reacting to itself. A null key is a clear() - // rather than a write, and it takes this key with it. + // This key only. The event does not fire in the tab that wrote it, so the + // setter below need not guard against itself. A null key is a clear(). if (event.key === null || event.key === THEME_STORAGE_KEY) { setChoiceState(readStoredChoice()); } @@ -135,7 +99,7 @@ export function useTheme() { useEffect(() => { // Both halves, because the inline script sets both before first paint and - // the one left unmaintained goes stale the first time the user chooses. + // either one left unmaintained goes stale the first time the reader picks. document.documentElement.dataset.theme = resolved; document.documentElement.style.colorScheme = resolved; }, [resolved]); @@ -145,8 +109,8 @@ export function useTheme() { try { if (next === "system") { - // A delete, not the word: the default has exactly one representation, - // and every other value the key could hold is already treated as absent. + // A delete, not the word: the default has one representation, the key + // not being there. localStorage.removeItem(THEME_STORAGE_KEY); } else { localStorage.setItem(THEME_STORAGE_KEY, next); From 3334827d25cdfed50f29c706f28869fc584030d3 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:08:21 -0700 Subject: [PATCH 12/20] docs(i18n): trim the comments in the data, api, locale and theme layers The catalogs carried the most prose and needed the least. The dataset ceiling sentence, the ?url warning, the formatter-cache reason and the six reachability records are all still there. --- src/api/getCities.ts | 31 ++------ src/data/worldcities/cities.ts | 138 ++++++--------------------------- src/i18n/catalogs/en.ts | 70 ++--------------- src/i18n/catalogs/es.ts | 18 +---- src/i18n/catalogs/fr.ts | 37 +-------- src/i18n/catalogs/index.ts | 15 +--- src/i18n/catalogs/pseudo.ts | 65 ++-------------- src/i18n/datasetErrorText.ts | 19 +---- src/i18n/format.ts | 49 ++---------- src/i18n/localeStore.ts | 64 +++------------ src/i18n/resolveLocale.ts | 90 ++++----------------- src/theme/resolveTheme.ts | 21 +---- 12 files changed, 92 insertions(+), 525 deletions(-) diff --git a/src/api/getCities.ts b/src/api/getCities.ts index 61a8f78..88ee483 100644 --- a/src/api/getCities.ts +++ b/src/api/getCities.ts @@ -1,14 +1,11 @@ import type { City } from "../data/worldcities/cities"; import { loadCities } from "../data/worldcities/cities"; -// Re-exported so no consumer's import path changes: the definition moved, the -// import site did not. +// Re-exported so no consumer's import path changed when the definition moved. export type { City }; -// The failure vocabulary reaches the rest of the tree through this seam too, so -// the loader keeps exactly one consumer. The application layer and the catalogs -// need the code to choose a sentence; neither has any business reaching past -// this module to get it. +// The failure vocabulary reaches the tree through this seam too, so the loader +// keeps exactly one consumer. export type { DatasetErrorCode } from "../data/worldcities/cities"; export { DATASET_ERROR_CODES, DatasetError } from "../data/worldcities/cities"; @@ -16,35 +13,23 @@ export interface GetCitiesParams { searchTerm?: string; } -/** - * Simulated network latency, in milliseconds. - */ +/** Simulated network latency, in milliseconds. */ const LATENCY_MS = 200; -/** - * Fake API that returns cities matching a search term against city name, ascii - * name, country name, or country code. Capital is rendered by the table but is - * not matched, because its values are classification codes rather than anything - * a reader searches for; the loader's key comment carries the reasoning. The - * dataset itself is downloaded once and cached, so a failed download is what - * rejects here. - */ +/** Matches a term against name, ascii name, country and country code. */ export async function getCities({ searchTerm = "", }: GetCitiesParams = {}): Promise { const all = await loadCities(); const needle = searchTerm.trim().toLowerCase(); - // The empty term returns a copy, so the module-scope cache cannot escape - // and both branches hand back an array the caller owns. Copying 50,250 - // references costs less than the latency below. + // The empty term returns a copy, so the module-scope cache cannot escape. const matched = needle ? all.filter((city) => city.searchKey.includes(needle)) : [...all]; - // The latency is applied to the filter rather than only to the download, so a - // cache-warm call still behaves like a network call and the container's - // debounce timing keeps its meaning. + // Applied to the filter as well as the download, so a cache-warm call still + // behaves like a network call and the debounce timing keeps its meaning. await new Promise((resolve) => { setTimeout(resolve, LATENCY_MS); }); diff --git a/src/data/worldcities/cities.ts b/src/data/worldcities/cities.ts index 0a82b44..ca65fff 100644 --- a/src/data/worldcities/cities.ts +++ b/src/data/worldcities/cities.ts @@ -1,18 +1,13 @@ -// The ?url suffix is load-bearing. A plain value import of the same file would -// compile the whole dataset back into the JavaScript bundle, with no visible -// error, and the app would then download it a second time as well. +// The ?url suffix is load-bearing: a plain value import would compile several +// megabytes of dataset into the JavaScript chunk with no visible error. import citiesUrl from "./cities.json?url"; /** * simplemaps.com "World Cities" basic database, v1.91.3, distributed under * CC BY 4.0. See license.txt. * - * The dataset ships as cities.json. Only the columns the City type needs are - * kept; lat, lng, iso2, and admin_name are dropped. Rows are ordered by - * descending population so the default view leads with the largest cities. - * - * Provenance is recorded in license.txt and in the README rather than here, so - * there is one account of how the committed bytes came to exist. + * Only the columns this type needs are kept, and rows are ordered by descending + * population. Provenance is recorded in license.txt and the README, not here. * * Upstream quirks preserved deliberately: * - 432 rows have no population and are recorded as 0. @@ -26,10 +21,8 @@ import citiesUrl from "./cities.json?url"; * translated column and a regenerated asset, which is a data pipeline rather * than an internationalization change. * - * That is stated here as well as in the README because this is the file a reader - * asking why a name is not translated is already looking at. The two copies are - * held together by a guard in src/toolchain.test.ts, so neither can be reworded - * on its own. + * Stated here as well as in the README, and held together by a guard in + * src/toolchain.test.ts so neither copy can be reworded on its own. */ export interface City { id: number; @@ -41,21 +34,7 @@ export interface City { population: number; } -/** - * The ways loading the dataset can fail, as a closed set of codes. - * - * A code rather than a message, because the message below is English and the - * reader may not be. The application layer turns a code into the sentence a - * reader sees, and this module still imports nothing but its own asset: the - * codes are its own vocabulary in the way the city type is. - * - * Eight of the nine are thrown here. The ninth is the container's own fallback - * for a rejection that carries no error at all, which cannot originate here - * because nothing here rejects with a non-error. - * - * A tuple rather than a bare union, so the catalog test can walk the set rather - * than restate it. - */ +/** The ways loading can fail. Codes, because the messages below are English. */ export const DATASET_ERROR_CODES = [ "notAnObject", "missingRows", @@ -71,18 +50,7 @@ export const DATASET_ERROR_CODES = [ /** Which failure a dataset error is. */ export type DatasetErrorCode = (typeof DATASET_ERROR_CODES)[number]; -/** - * A dataset failure, carrying the code that says which one it is. - * - * The message stays exactly what it was and stays English: it is the - * developer-facing text, the one a stack trace and a test assertion read. The - * preserved cause stays where it was attached. Neither reaches the screen. - * - * The detail is a single number and every failure carries one, including the - * six whose sentence ignores it. Uniform on purpose, so the lookup that turns a - * code into a sentence has one shape and no branch; the three that use it carry - * a row index or a response status. - */ +/** A dataset failure and its code. The message never reaches the screen. */ export class DatasetError extends Error { constructor( readonly code: DatasetErrorCode, @@ -95,11 +63,7 @@ export class DatasetError extends Error { } } -/** - * The column order the asset must declare. Asserting it is what converts the - * tuple shape's one real defect from silently mis-mapping 50,250 rows into a - * loud startup failure. - */ +/** The order the asset must declare, so a mis-mapping is a startup failure. */ const COLUMNS = [ "id", "name", @@ -110,42 +74,18 @@ const COLUMNS = [ "population", ] as const; -/** - * Separates the fields of the derived search key. A text input cannot produce - * this character, so a needle can never match across a field boundary. - * Concatenating with a space instead would diverge from the per-field matcher - * this replaces on thousands of rows. - */ +/** A text input cannot produce this, so a needle cannot cross a field. */ const SEARCH_KEY_SEPARATOR = "\u0000"; -/** - * The derived key is a search cache, not a fact about a city, so it stays off - * the exported type: a searchKey on City would surface in a column descriptor - * as a column-shaped field that is not a column. - */ +/** A search cache rather than a fact about a city, so it stays off City. */ interface IndexedCity extends City { searchKey: string; } -/** - * How long the whole download gets before a stall counts as a failure. Without - * it a stalled request never rejects and the reader waits on a spinner with - * nothing behind it. - * - * Deliberately generous, because this is a wall-clock deadline over a 3.3MB - * asset with no resume: a retry restarts the download from zero against the - * same budget, so a link too slow to finish inside it fails every attempt - * rather than merely being slow. A minute clears roughly 140kbps sustained, - * which is below any link that could have finished before this existed. - */ +/** Generous: a wall-clock deadline over a 3.3MB asset that cannot resume. */ const LOAD_TIMEOUT_MS = 60_000; -/** - * A download that did not finish, whichever way it stopped. A stall and a - * dropped connection are one failure to a reader, whose next move is the retry - * either way, so they share a code and a sentence rather than splitting the - * catalog. - */ +/** A download that did not finish. A stall and a drop are one failure. */ function transportError(cause: unknown): DatasetError { return new DatasetError( "transport", @@ -157,12 +97,7 @@ function transportError(cause: unknown): DatasetError { let cached: Promise | undefined; -/** - * The only place the untyped result of response.json() is narrowed. Each - * failure carries a code, and the sentence a reader is shown is chosen from - * that code one layer up; the message written here is the developer-facing - * text and no longer reaches the screen. - */ +/** The only place the untyped result of response.json() is narrowed. */ function parseCities(payload: unknown): IndexedCity[] { if (typeof payload !== "object" || payload === null) { throw new DatasetError( @@ -230,12 +165,8 @@ function parseCities(payload: unknown): IndexedCity[] { countryIso3, capital, population, - // Four of the five rendered columns. Capital is the one left out, and - // deliberately: its only values are the upstream classification codes - // "primary", "admin", "minor", and empty, which nobody types into a city - // search, and folding them in would bleed into every short needle. Over - // the committed rows "in" would match 31,388 of 50,250 instead of 19,051, - // through "admin" and "minor" rather than through anything a reader meant. + // Four of the five rendered columns. Capital is left out: its upstream + // codes would make "in" match 31,388 rows of 50,250 instead of 19,051. searchKey: [name, nameAscii, country, countryIso3] .join(SEARCH_KEY_SEPARATOR) .toLowerCase(), @@ -243,23 +174,15 @@ function parseCities(payload: unknown): IndexedCity[] { }); } -/** - * Downloads, validates, and indexes the dataset, caching the promise at module - * scope. A cache hit returns the same promise, which is what makes a double - * mount issue one request by construction rather than by cancellation. - */ +/** The module-scope cache is what makes a double mount issue one request. */ export function loadCities(): Promise { if (cached) return cached; const pending = fetch(citiesUrl, { signal: AbortSignal.timeout(LOAD_TIMEOUT_MS), }) - // The text a request carries when it never reaches the host is the - // browser's own, it differs between browsers, and none of it tells the - // reader what to do. It is replaced here and kept as the cause. This is - // attached to the request rather than to the chain below, so nothing - // thrown while reading the response can be reported as a transport - // failure. + // The browser's own text tells the reader nothing, so it is replaced and + // kept as the cause. Attached here, so a read failure is not reported as one. .catch((reason: unknown) => { throw transportError(reason); }) @@ -271,19 +194,11 @@ export function loadCities(): Promise { `The city data could not be downloaded (status ${response.status}).`, ); } - // A static host that serves the application's own page for a file it - // cannot find answers with a success status and a body of HTML, so the - // parser reports a syntax error naming a character rather than anything - // the reader can act on. The status check stays ahead of this, so a - // status failure is never reported as a parse failure. + // A static host serving its own page for a missing file answers with a + // success status and HTML, so the status check stays ahead of this. return response.json().catch((reason: unknown) => { // A body that is not JSON fails the parse and nothing else does, so - // the parse failure is the narrow case and everything else here is the - // download stopping partway: the timeout covers the body read, and a - // socket dropped after the headers arrived rejects here too. Both are - // failed downloads, and reporting either as an unreadable file would - // send the reader looking for a corrupt asset instead of at their - // connection. + // everything else reaching here is the download stopping partway. if (!(reason instanceof SyntaxError)) { throw transportError(reason); } @@ -297,13 +212,8 @@ export function loadCities(): Promise { }) .then(parseCities); - // Attached at store time and never deferred. Any delay leaves a window in - // which a retry re-awaits the already-rejected promise and fails instantly - // for a reason the user cannot see. The clear is unconditional because the - // only path that stores a new entry runs after this handler has already - // cleared the old one, so a rejection can never reach an entry other than - // its own. The rejection still reaches callers, who hold the promise - // returned below rather than this derived one. + // Attached at store time: any delay leaves a window in which a retry + // re-awaits the already-rejected promise. pending.catch(() => { cached = undefined; }); diff --git a/src/i18n/catalogs/en.ts b/src/i18n/catalogs/en.ts index da28c84..9d1e606 100644 --- a/src/i18n/catalogs/en.ts +++ b/src/i18n/catalogs/en.ts @@ -2,60 +2,25 @@ import type { DatasetErrorCode } from "../../api/getCities"; import { numberFormatFor, selectPlural } from "../format"; /** - * The nouns the two woven sentences below pluralize, one record per noun, - * total over the two categories the English tag reports. - * - * Total rather than defaulted, so there is no fallback arm here that nothing - * can reach and no branch the coverage gate cannot cover. The category set is - * CLDR data rather than a type, so what holds these honest is the catalog test, - * which calls every catalog with a count drawn from each of its own tag's - * categories and reads the result for a hole. + * The nouns the woven sentences pluralize, total over the two categories + * English reports, so there is no branch the coverage gate cannot cover. */ const CITY = { one: "city", other: "cities" }; const RESULT = { one: "result", other: "results" }; const ENTRY = { one: "entry", other: "entries" }; -/** - * Which way a sorted column runs. - * - * Declared here rather than imported from the table, because a catalog names - * words and must not learn what a table is. It is the same pair of tokens the - * sort state travels as, and it arrives at the two entries below as a value - * precisely so that no sentence has to build a word out of it. - */ +/** Declared here rather than imported: a catalog must not learn about tables. */ export type SortedDirection = "asc" | "desc"; -/** - * The two directions, as the words the sentences below weave in. - * - * A record rather than a suffix appended to the token, and the difference is - * the whole reason this file changed: "ascend" plus "ing" is a word in exactly - * one language, and a sentence assembled that way cannot be translated at all. - * Every language spells the pair out here and reads it by key. - */ +/** A record rather than a suffix: "ascend" plus "ing" is one language only. */ const DIRECTION: Readonly> = { asc: "ascending", desc: "descending", }; /** - * What a reader is told when the city data cannot be loaded, one sentence per - * failure code. - * - * Total over the code union rather than defaulted, so a code added to the loader - * without a sentence in all four catalogs fails the type check instead of - * rendering the word undefined at the moment the application has already - * failed. There is no fallback arm here and so no branch the coverage gate - * cannot reach. - * - * Every entry has the same signature and most ignore both arguments, which is - * what keeps the lookup one call with no branch. The three that use the second - * one weave a row index or a response status and group it on the resolved tag, - * like every other number this application shows. - * - * The field count in the two row sentences is written out rather than passed - * in. It is a fact about the asset's shape rather than a quantity a reader's - * locale groups, and the detail slot is already carrying the row. + * One sentence per failure code, total over the code union, so a new code fails + * the type check and no branch is left the coverage gate cannot reach. */ export type DatasetErrorText = Readonly< Record string> @@ -78,21 +43,7 @@ const DATASET_ERROR_TEXT: DatasetErrorText = { unexpected: () => "An unexpected error occurred.", }; -/** - * The base catalog: every string the city table shows that names what its rows - * are or what a column of them holds, in the language the rest of the tree is - * checked against. - * - * The wording is the wording the table already shipped, up to the two nouns - * that now follow their count and the two counts that are now grouped. - * - * The entries taking the resolved language tag as their first parameter take it - * for one reason: the count a reader sees is grouped by that tag's own rule, and - * the noun beside it is selected over the categories the tag reports. Neither - * decision can be made where the sentence is assembled, because that is one - * layer below the locale by construction. An entry needing neither takes no tag, - * so the signature says which entries are locale-sensitive and which are copy. - */ +/** An entry takes the tag only when it groups a number or picks a plural. */ export const en = { appTitle: "City List", themeGroup: "Theme", @@ -145,10 +96,5 @@ export const en = { }, }; -/** - * The shape every other catalog is held to. Derived from the base rather than - * declared beside it, so the key set has one definition: a catalog missing a key - * or misspelling one fails the type check rather than rendering undefined at a - * reader. - */ +/** Derived from the base, so a missing key in another catalog fails to type. */ export type Catalog = typeof en; diff --git a/src/i18n/catalogs/es.ts b/src/i18n/catalogs/es.ts index b67f4bc..376b762 100644 --- a/src/i18n/catalogs/es.ts +++ b/src/i18n/catalogs/es.ts @@ -2,14 +2,8 @@ import { numberFormatFor, selectPlural } from "../format"; import type { Catalog, DatasetErrorText, SortedDirection } from "./en"; /** - * The nouns the two woven sentences below pluralize, total over the three - * categories the Spanish tag reports. - * - * The many form is spelled out rather than shared with the other form, even - * though Spanish inflects them the same way. The category exists because - * Spanish treats round millions differently in compact notation, and writing - * the arm out is what makes the record total by construction rather than by a - * reader remembering that two of the three happen to agree today. + * Total over the three categories Spanish reports. The many form is spelled + * out, which makes the record total by construction. */ const CIUDAD = { one: "ciudad", many: "ciudades", other: "ciudades" }; const RESULTADO = { one: "resultado", many: "resultados", other: "resultados" }; @@ -41,13 +35,7 @@ const TEXTO_DE_ERROR: DatasetErrorText = { unexpected: () => "Se produjo un error inesperado.", }; -/** - * The Spanish catalog. - * - * Declared with satisfies rather than annotated with it, so a missing key and a - * misspelled key are both compile errors while the literal types of the entries - * survive for anything that wants to read them. - */ +/** Declared with satisfies, so a missing key is a compile error. */ export const es = { appTitle: "Lista de ciudades", themeGroup: "Tema", diff --git a/src/i18n/catalogs/fr.ts b/src/i18n/catalogs/fr.ts index 1a44c9f..ec183e3 100644 --- a/src/i18n/catalogs/fr.ts +++ b/src/i18n/catalogs/fr.ts @@ -1,15 +1,7 @@ import { numberFormatFor, selectPlural } from "../format"; import type { Catalog, DatasetErrorText, SortedDirection } from "./en"; -/** - * The nouns the two woven sentences below pluralize, total over the three - * categories the French tag reports. - * - * French puts zero in the singular category where English and Spanish put it in - * the plural, so "0 ville" is correct here and "0 ciudades" is correct beside - * it. That is the rule a ternary on the count gets wrong without ever looking - * wrong, and it is the reason the selection goes through the platform. - */ +/** Total over three categories. French puts zero in the singular. */ const VILLE = { one: "ville", many: "villes", other: "villes" }; const RESULTAT = { one: "résultat", many: "résultats", other: "résultats" }; const ENTREE = { one: "entrée", many: "entrées", other: "entrées" }; @@ -20,17 +12,7 @@ const ORDRE: Readonly> = { desc: "décroissant", }; -/** - * The narrow no-break space French typography sets before a colon, a semicolon, - * an exclamation mark and a question mark. - * - * Written as an escape rather than as the character, because the character is - * indistinguishable from an ordinary space in every editor and terminal this - * file is read in, and a reviewer meeting it inline would correct it. It is a - * translation requirement, not a typo. The same character is already in every - * grouped number these entries carry, put there by the formatter rather than by - * hand. - */ +/** U+202F, as an escape: inline it is indistinguishable from a space. */ const NARROW_NO_BREAK_SPACE = "\u202F"; /** Ce qui est annoncé au lecteur quand les données ne peuvent pas être chargées. */ @@ -52,20 +34,7 @@ const TEXTE_ERREUR: DatasetErrorText = { unexpected: () => "Une erreur inattendue s'est produite.", }; -/** - * The French catalog. - * - * Declared with satisfies rather than annotated with it, so a missing key and a - * misspelled key are both compile errors while the literal types of the entries - * survive for anything that wants to read them. - * - * French typography puts a narrow no-break space, U+202F, before a colon, a - * semicolon, an exclamation mark and a question mark. Two entries below carry - * one: the label above the page-size control and the prefix on a failure. Both - * reach it through the named constant above rather than by holding the - * character inline, so a reviewer reads the requirement instead of a space that - * looks like a typo. - */ +/** Declared with satisfies, so a missing key is a compile error. */ export const fr = { appTitle: "Liste des villes", themeGroup: "Thème", diff --git a/src/i18n/catalogs/index.ts b/src/i18n/catalogs/index.ts index d85476e..eccf495 100644 --- a/src/i18n/catalogs/index.ts +++ b/src/i18n/catalogs/index.ts @@ -4,13 +4,7 @@ import { es } from "./es"; import { fr } from "./fr"; import { pseudo } from "./pseudo"; -/** - * Every catalog, by id. - * - * Total over the closed union rather than looked up with a fallback, so a value - * that somehow reached here unchecked still cannot find a missing arm, and a - * catalog added later cannot be forgotten here without failing the type check. - */ +/** Every catalog, by id, and total, so a new one cannot be forgotten here. */ export const CATALOGS: Readonly> = { en, es, @@ -18,12 +12,7 @@ export const CATALOGS: Readonly> = { "ar-XB": pseudo, }; -/** - * What each catalog calls itself, written in its own language and never - * translated. A reader who cannot read the interface they are looking at has to - * be able to find their own language in the picker, which is the whole job of - * this record and the reason there is exactly one literal per id. - */ +/** What each catalog calls itself, in its own language, never translated. */ export const AUTONYMS: Readonly> = { en: "English", es: "Español", diff --git a/src/i18n/catalogs/pseudo.ts b/src/i18n/catalogs/pseudo.ts index d84797d..638650d 100644 --- a/src/i18n/catalogs/pseudo.ts +++ b/src/i18n/catalogs/pseudo.ts @@ -5,39 +5,9 @@ import { type SortedDirection, } from "./en"; -/** - * The right-to-left pseudo-locale. - * - * It exists so the direction and the truncation can be tested, because the - * three catalogs beside it all read left to right and would leave both - * untestable. - * - * A deliberate hybrid of the two pseudo-locales the browsers already ship: the - * direction comes from the right-to-left one, the readability and the padding - * come from the left-to-right one. The character reversal the real right-to-left - * pseudo-locale performs is dropped on purpose: anyone reading this repository - * has to be able to read this catalog, which is why it was taken over shipping - * Arabic strings nobody here can review. - * - * Every entry is derived from the corresponding entry of the base catalog rather - * than committed as a transformed literal, so this file cannot drift from the - * copy it pseudo-translates. - * - * The two bidirectional control characters this file needs are written as - * escapes rather than as glyphs. They are invisible either way, and a raw one - * in source is the shape a hidden-character attack takes, so tooling flags it - * and is right to. The escape says the same thing in characters a reviewer can - * see. - */ +/** The pseudo-locale, so direction and truncation have something to prove. */ -/** - * Opens a run whose direction is taken from its first strongly directional - * character, so a Latin string renders as its own left-to-right run inside a - * right-to-left document instead of having its punctuation scattered. - * - * An isolate rather than a directional mark, because a mark states a direction - * at a point and cannot bound a run. - */ +/** An isolate, not a mark: a mark cannot bound a run, only start one. */ const FIRST_STRONG_ISOLATE = "\u2066"; /** Closes the run the isolate above opened. */ @@ -46,30 +16,14 @@ const POP_DIRECTIONAL_ISOLATE = "\u2069"; /** What the padding is made of. Visibly filler, so nobody reads it as copy. */ const PADDING_CHARACTER = "~"; -/** - * One message, pseudo-translated. - * - * Three things at once, each catching a different defect. The brackets bound the - * message unit, so a sentence assembled out of two catalog entries shows up as - * two bracketed units rather than as one plausible line. The isolates keep the - * readable run readable inside a right-to-left document. The padding grows the - * string by roughly a third, which is about what a real translation costs, so a - * layout that truncates or overflows does it here rather than in front of a - * reader. - */ +/** The brackets bound the unit; the padding is what a translation costs. */ export function pseudoize(message: string): string { const padding = PADDING_CHARACTER.repeat(Math.ceil(message.length / 3)); return `[${FIRST_STRONG_ISOLATE}${message}${POP_DIRECTIONAL_ISOLATE} ${padding}]`; } -/** - * The dataset failure sentences, each derived from the base catalog's own so - * this record cannot drift from the copy it pseudo-translates. Written out - * entry by entry like every other entry in this file, rather than built from - * the code tuple, because a construction would need a cast to be typed and the - * cast is what would hide a missing arm. - */ +/** Entry by entry, because a built record would need a cast to be typed. */ const DATASET_ERROR_TEXT: DatasetErrorText = { notAnObject: (tag, detail) => pseudoize(en.datasetError.notAnObject(tag, detail)), @@ -87,16 +41,7 @@ const DATASET_ERROR_TEXT: DatasetErrorText = { pseudoize(en.datasetError.unexpected(tag, detail)), }; -/** - * The pseudo-locale catalog. Every function-valued entry pseudo-translates the - * base catalog's result rather than a template, so the values woven into a - * sentence land inside the brackets where a truncation would cut them. - * - * It declares no plural nouns of its own, and that is not an omission. Its - * strings really are English and its resolved tag really is the English one, so - * the base catalog's two categories are its two categories, and a second set - * here could only ever drift from them. - */ +/** Each entry translates the base result, so woven values stay bracketed. */ export const pseudo = { appTitle: pseudoize(en.appTitle), themeGroup: pseudoize(en.themeGroup), diff --git a/src/i18n/datasetErrorText.ts b/src/i18n/datasetErrorText.ts index ee40e8e..f5037d5 100644 --- a/src/i18n/datasetErrorText.ts +++ b/src/i18n/datasetErrorText.ts @@ -3,22 +3,9 @@ import { DatasetError } from "../api/getCities"; import type { Catalog } from "./catalogs/en"; /** - * The sentence a reader is shown for a failed load. - * - * Called during render rather than where the failure is caught, and the - * difference matters: the catch lives inside the container's fetch effect, so - * reading the catalog there would put the locale in that effect's dependency - * array and changing the language would re-issue the search. - * - * The lookup is total over a closed union of codes read off a class instance, - * so nothing a reader controls indexes it and there is no fallback arm inside - * it. Only the selected sentence is returned; the failure's own message is - * developer-facing text and its preserved cause is engine text, and neither is - * something a reader was ever meant to see. - * - * A failure that is not a dataset error is one this application has no code - * for, and it takes the unexpected sentence. That is the second of the two - * branches here, and it is the one a rejection carrying no error at all reaches. + * The sentence a reader is shown for a failed load. Called during render, not + * at the catch, which would put the locale in the fetch effect's dependencies. + * The second of the two branches is the one a rejection with no error reaches. */ export function datasetErrorText( error: Error, diff --git a/src/i18n/format.ts b/src/i18n/format.ts index c551cd5..35ef8ac 100644 --- a/src/i18n/format.ts +++ b/src/i18n/format.ts @@ -1,35 +1,16 @@ -// The one module in the tree that asks the platform for a locale. Every -// locale-sensitive decision the application makes reaches a constructor here, -// keyed by the resolved tag, and a guard in src/toolchain.test.ts fails if a -// second module grows one of its own. +// The one module in the tree that asks the platform for a locale. A guard in +// src/toolchain.test.ts fails if a second module grows a constructor of its own. /** - * One instance per tag, held for the module's lifetime. - * - * The reasoning moved here from the single module-scope collator this replaces, - * and it survives the move intact: building a collator inside the comparison - * would build roughly eight hundred thousand of them for a single sort of the - * full dataset. The value formatter has the same shape of cost one layer up, - * because the platform's per-value formatting helper builds a formatter on - * every call, which is one per rendered cell per render. - * - * What changed is that the instance can no longer be a constant: it is a - * function of the resolved tag, and the tag moves when the reader chooses. A - * map keyed by tag is what keeps one instance per locale rather than one - * instance per document, and it is a cache with a known ceiling rather than one - * that grows with input, because the only keys reaching it are the tags of a - * four-entry frozen record. + * One instance per tag, held for the module's lifetime. A collator built inside + * a comparison would be roughly eight hundred thousand of them for one sort of + * the full dataset. Keyed by tag, so the ceiling is the catalog count. */ const collators = new Map(); const numberFormats = new Map(); const pluralRules = new Map(); -/** - * The one lookup the three below share, written once rather than three times. - * Absence is tested rather than truthiness because the value is an object and - * the map holds no falsy ones, so the two would agree today and diverge the - * first time something cacheable is not. - */ +/** The one lookup the three below share. Absence, not truthiness. */ function cached( cache: Map, tag: string, @@ -61,22 +42,8 @@ export function pluralRulesFor(tag: string): Intl.PluralRules { } /** - * Picks a noun's form for a count, over the categories the tag itself reports - * rather than over a singular-or-other pair. - * - * The pair is wrong in half the catalogs that ship: Spanish and French each - * report three categories where English reports two, and French puts zero in - * the singular where the other two put it in the plural. A ternary on the count - * cannot express either of those, and would be silently wrong rather than - * visibly missing. - * - * The assertion on the selected category is the one place in this module where - * the platform's answer is narrowed to what the caller declared, and it is - * sound exactly while a caller's record is total over the categories its own - * tag reports. Nothing in the type system can check that, because the category - * set is CLDR data rather than a type, so the catalog test asserts it directly: - * every catalog is called with a count drawn from each of its tag's categories - * and its sentences are read for a hole. + * Picks a noun's form over the categories the tag reports. A + * singular-or-other pair is wrong in half the shipped catalogs. */ export function selectPlural( tag: string, diff --git a/src/i18n/localeStore.ts b/src/i18n/localeStore.ts index beb529c..47e712f 100644 --- a/src/i18n/localeStore.ts +++ b/src/i18n/localeStore.ts @@ -7,26 +7,9 @@ import { type ResolvedLocale, } from "./resolveLocale"; -/** - * The chosen locale, owned once for the whole document. - * - * The theme's choice lives inside its hook, one copy per caller, and its own - * documentation says that is why a second caller would race the first. This one - * has several callers by design: the picker in the header and the table below - * it both read it, and both stamp the same document element. Holding the choice - * here rather than in the hook is what makes those writes agree by - * construction, because every subscriber resolves the identical value. - */ +/** Owned once for the document, which is what makes several callers agree. */ -/** - * Reads the stored choice. Anything that is not the id of a catalog that ships - * is absent, and so is a store that cannot be read at all: a stale entry from an - * older build and a hostile one are the same case, and both resolve to - * following the machine rather than to an undefined locale. - * - * The property access is what throws when site data is blocked, so neither a - * typeof guard nor optional chaining substitutes for the catch. - */ +/** Anything not a shipped id is absent. The access throws when blocked. */ function readStoredChoice(): LocaleChoice { try { const stored = localStorage.getItem(LOCALE_STORAGE_KEY); @@ -52,15 +35,7 @@ function notify(): void { } } -/** - * Another document wrote the key. Re-read the store rather than trusting the - * value the event carries, and ignore every other key: the event does not fire - * in the document that made the write, which is why the setter below does not - * have to guard against reacting to itself. - * - * A null key is a clear() rather than a write, and it takes this key with it, so - * it counts the same as a write to this key. - */ +/** Re-read rather than trust the event. A null key is a clear(). */ function handleStorage(event: StorageEvent): void { if (event.key === null || event.key === LOCALE_STORAGE_KEY) { choice = readStoredChoice(); @@ -68,16 +43,7 @@ function handleStorage(event: StorageEvent): void { } } -/** - * Registers a reader and returns the unregister. - * - * The window listeners are installed with the first subscriber and removed with - * the last, so a document with nothing mounted holds nothing. That is also why - * the choice is re-read here: between the last unsubscribe and this call there - * was no listener, so a write from another document in that window went unseen. - * React re-reads the snapshot immediately after subscribing, which is what turns - * the re-read into a render rather than into a value nobody asked for. - */ +/** The listeners come and go with the first and last subscriber. */ export function subscribeLocale(onStoreChange: () => void): () => void { if (subscribers.size === 0) { choice = readStoredChoice(); @@ -97,14 +63,7 @@ export function subscribeLocale(onStoreChange: () => void): () => void { }; } -/** - * The resolved locale right now. - * - * Safe to hand straight to React with no cache in front of it, because the - * resolver returns one of four module constants and so its identity is already - * stable for a stable input. A resolver that built its answer would need one, - * and the loop it would otherwise cause is the reason this is worth stating. - */ +/** Safe to hand straight to React: the resolver returns a module constant. */ export function getLocaleSnapshot(): ResolvedLocale { return resolveLocale(choice, navigator.languages); } @@ -115,13 +74,8 @@ export function getChoiceSnapshot(): LocaleChoice { } /** - * Moves the choice and writes it through. - * - * Takes a string rather than the union because the picker hands over whatever - * the DOM has in the control's value, and this is the one place that decides - * whether a value names a choice. A value that names none is ignored outright: - * the option list is closed, so nothing that reaches this branch came from a - * reader picking something. + * Moves the choice and writes it through. A value naming none is ignored: the + * option list is closed, so nothing reaching that branch came from a reader. */ export function setLocaleChoice(next: string): void { if (!isLocaleChoice(next)) { @@ -132,8 +86,8 @@ export function setLocaleChoice(next: string): void { try { if (next === "system") { - // A delete, not the word: the default has exactly one representation, and - // every other value the key could hold is already treated as absent. + // A delete, not the word: the default has one representation, the key + // not being there. localStorage.removeItem(LOCALE_STORAGE_KEY); } else { localStorage.setItem(LOCALE_STORAGE_KEY, next); diff --git a/src/i18n/resolveLocale.ts b/src/i18n/resolveLocale.ts index ab6a707..ab4c58f 100644 --- a/src/i18n/resolveLocale.ts +++ b/src/i18n/resolveLocale.ts @@ -1,52 +1,26 @@ -// The locale's vocabulary and the one rule that turns a stored choice plus the -// reader's own preferences into something the document, the catalogs and the -// platform formatters can each act on. Everything the inline script in +// The locale's vocabulary and the rule that turns a stored choice plus the +// reader's preferences into a resolved locale. Everything the inline script in // index.html duplicates by hand is declared here. -/** - * Every catalog that ships, in the order the picker offers them. - * - * The last is not a language. It is the readable right-to-left pseudo-locale - * that exists so direction and truncation have something to prove themselves - * against, because the other three are all left to right. - */ +/** Every catalog that ships. The last is a pseudo-locale, not a language. */ export const CATALOG_IDS = ["en", "es", "fr", "ar-XB"] as const; /** The literal union of the ids above, formed with no assertion anywhere. */ export type CatalogId = (typeof CATALOG_IDS)[number]; /** - * The catalogs a preference list is allowed to select. - * - * The pseudo-locale is excluded because its primary subtag is ar, and an - * unfiltered walk would hand a reader who genuinely prefers Arabic a catalog of - * bracketed English. It stays reachable the only way it should be, by being - * chosen. + * The catalogs a preference list may select. The pseudo-locale is excluded, so + * it stays reachable the only way it should be, by being chosen. */ export const NEGOTIABLE_CATALOG_IDS = ["en", "es", "fr"] as const; -/** - * What the reader picked. The word rather than a catalog id means follow the - * machine, which is the default and is a choice like any other rather than the - * absence of one. - */ +/** The word rather than an id means follow the machine, the default. */ export type LocaleChoice = CatalogId | "system"; /** The storage key. The inline script in index.html spells this out by hand. */ export const LOCALE_STORAGE_KEY = "yart-locale"; -/** - * A resolved locale is three fields rather than one: which catalog supplies the - * strings, which language tag the platform formatters get, and which direction - * the document carries. - * - * The split is load-bearing rather than tidy. The pseudo-locale's id is a well - * formed language tag, so an engine carrying Arabic data would collate and - * format for Arabic if the id were passed through, and jsdom, Chromium and Node - * would each answer differently for reasons that have nothing to do with this - * code. Its strings really are English, so its tag is English and only its - * direction is borrowed. - */ +/** Three fields, because the pseudo-locale borrows a direction, not a tag. */ export interface ResolvedLocale { /** Which catalog supplies the strings. */ readonly catalog: CatalogId; @@ -56,18 +30,7 @@ export interface ResolvedLocale { readonly dir: "ltr" | "rtl"; } -/** - * The whole mapping, as literals. - * - * Direction is written out rather than asked of Intl.Locale.prototype - * getTextInfo, which is Chrome 130, Firefox 153 and Safari 17 against this - * app's floor of Chrome 111, Firefox 111 and Safari 16.4. Node 24 carries it, - * so a test of it under this runner would pass while Firefox 111 through 152 - * threw at the reader. - * - * Indexed only by a value of the closed union, so the lookup is total and there - * is no fallback arm here that nothing can reach. - */ +/** Direction written out: Intl.Locale getTextInfo is above the browser floor. */ const RESOLVED_LOCALES = { en: { catalog: "en", tag: "en-US", dir: "ltr" }, es: { catalog: "es", tag: "es-ES", dir: "ltr" }, @@ -75,15 +38,9 @@ const RESOLVED_LOCALES = { "ar-XB": { catalog: "ar-XB", tag: "en-US", dir: "rtl" }, } as const satisfies Readonly>; -/** - * Whether a value names a catalog that ships. The one gate between a - * reader-controlled string, from storage or from another document, and a lookup - * in a record keyed by the closed union. - */ +/** The one gate between a reader-controlled string and the closed union. */ export function isCatalogId(value: unknown): value is CatalogId { - // Widened for the search alone. The array is a closed tuple of catalog ids, - // so its own includes rejects an unknown argument outright, and the whole - // point here is to ask about one. + // Widened for the search: the tuple's own includes rejects an unknown. return (CATALOG_IDS as readonly unknown[]).includes(value); } @@ -93,10 +50,8 @@ export function isLocaleChoice(value: unknown): value is LocaleChoice { } /** - * A language tag's primary subtag, lowercased, which is the unit the lookup - * below matches on. Written with a search rather than a split so a tag carrying - * no separator takes a real branch instead of an index access that can never be - * absent. + * A language tag's primary subtag. A search rather than a split, so a tag with + * no separator takes a real branch rather than an index that is never absent. */ function primarySubtag(tag: string): string { const separator = tag.indexOf("-"); @@ -105,24 +60,9 @@ function primarySubtag(tag: string): string { } /** - * The rule that turns a choice plus the reader's preference list into the - * locale everything downstream reads. - * - * An explicit choice wins outright. Otherwise the preferences are walked in - * order and the first one whose primary subtag names a negotiable catalog is - * taken, which is the lookup rule from the language-tag matching standard - * reduced to the part this app can answer. Nothing matching is the base catalog - * rather than a failure, so the function is total and no caller has a throw to - * handle. - * - * Every answer is one of four module constants, so the same input returns the - * same object identity. The store below depends on that: it is what lets a - * snapshot reader be handed straight to React without a cache in front of it. - * - * This same rule is written a second time, as a literal, inside the blocking - * inline script in index.html. It has to be: that script runs before any module - * loads, so it cannot import this function. The guard in src/toolchain.test.ts - * is what holds the two copies together. + * Turns a choice plus a preference list into a resolved locale, always one of + * four module constants, so identity is stable. Written a second time as a + * literal in index.html's inline script; the parity guard holds the two. */ export function resolveLocale( choice: LocaleChoice, diff --git a/src/theme/resolveTheme.ts b/src/theme/resolveTheme.ts index 1d70b2c..6208733 100644 --- a/src/theme/resolveTheme.ts +++ b/src/theme/resolveTheme.ts @@ -2,14 +2,7 @@ // selector can match. Everything the inline script in index.html duplicates by // hand is declared here. -/** - * Every state the theme control offers, in the order it offers them. - * - * A value rather than a bare union, so the accepted set has one definition that - * both the hook's stored-choice check and the parity guard in - * src/toolchain.test.ts can read. The locale's own vocabulary derives its union - * from a tuple the same way, for the same reason. - */ +/** A value rather than a bare union, so the accepted set has one definition. */ export const THEME_CHOICES = ["light", "dark", "system"] as const; /** The literal union of the words above, formed with no assertion anywhere. */ @@ -24,15 +17,9 @@ export const THEME_STORAGE_KEY = "yart-theme"; export const PREFERS_DARK_QUERY = "(prefers-color-scheme: dark)"; /** - * The rule that turns a stored choice plus the operating system's preference into - * the concrete theme the document element carries. The attribute is never the - * word "system": that is a choice, not a theme, and a selector cannot resolve it. - * - * This same rule is written a second time, as a literal, inside the blocking - * inline script in index.html. It has to be: that script runs before any module - * loads, so it cannot import this function. A change to either one needs the - * same change to the other, and the parity guard in src/toolchain.test.ts is - * what fails when they stop agreeing. + * Turns a choice plus the system preference into the theme the document element + * carries; the attribute is never "system". Written a second time as a literal + * in index.html's inline script, and the parity guard holds the two together. */ export function resolveTheme( choice: ThemeChoice, From 512bc41d0f70e36074ab671df55da42e3b962ce8 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:14:11 -0700 Subject: [PATCH 13/20] docs(app): trim the comments in the feature layer and the app root Comment lines across non-test source fall from 1682 of 3979 to 505 of 2802, and comment runs longer than three lines from 164 to 32. The replaceState reason, the address invariant the cross-document guard holds, and every a11y and reachability record are still in place. --- src/App.tsx | 45 ++---- src/appState.ts | 51 ++----- src/components/DataTable/DataTable.tsx | 5 +- src/components/DataTable/sortRows.ts | 10 +- src/components/DataTable/tableState.ts | 5 +- src/components/DataTable/tableStateUrl.ts | 5 +- src/components/paginate.ts | 5 +- src/features/CityTable/CityTable.tsx | 166 ++++++---------------- src/features/CityTable/cityColumns.ts | 55 ++----- src/features/CityTable/cityLabels.ts | 26 +--- src/features/CityTable/index.ts | 6 +- src/features/Footer/Footer.tsx | 31 ++-- src/features/Header/LocaleControl.tsx | 22 +-- src/features/Header/ThemeControl.tsx | 33 ++--- src/features/RootLayout/ErrorBoundary.tsx | 25 +--- src/features/RootLayout/RootLayout.tsx | 6 +- src/hooks/useDebouncedCallback.ts | 5 +- src/hooks/useLocale.ts | 5 +- src/hooks/useTheme.ts | 10 +- src/i18n/format.ts | 5 +- 20 files changed, 126 insertions(+), 395 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index f06467a..93832ae 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -9,15 +9,8 @@ import { RootLayout } from "./features/RootLayout"; import { CityTable, parseSearchTerm } from "./features/CityTable"; const App = () => { - // Seeded from the address so the first render already asks for the right - // rows. Without this the effect below issues a request for the empty term and - // then immediately another for the restored one, so every shared link costs - // two requests on a cold start; the stale-result guard makes that survivable - // rather than correct. - // - // This is a read of the address and stays one. The single write lives with - // the table's view state, one layer down, and adding a second writer here is - // what would make the address a thing two components argue over. + // Seeded from the address, or every shared link costs two requests on a cold + // start. A read and it stays one: the single write is one layer down. const [searchTerm, setSearchTerm] = useState(() => parseSearchTerm(window.location.search), ); @@ -26,11 +19,8 @@ const App = () => { const { catalog, tag } = useLocale(); useEffect(() => { - // The asynchronous work sits directly in the effect rather than behind a - // memoized callback, because that is what lets this one variable guard - // every state write below, the settle handler included. A result that - // arrives after the cleanup has run belongs to a search the user has - // already moved past, so it is dropped rather than rendered. + // The fetch sits in the effect so this one flag guards every write below. + // A result arriving after cleanup belongs to a search already moved past. let ignore = false; // Clear the last failure as the new attempt starts, so a retry does not @@ -47,10 +37,8 @@ const App = () => { if (err instanceof Error) { dispatch({ type: "failed", error: err }); } else { - // A rejection carrying no error at all, which the loader never - // produces and a stubbed seam can. It enters state as a dataset - // error so what the reducer holds is always something the translator - // below has a sentence for. + // A rejection carrying no error, which only a stubbed seam produces. + // It enters state as a dataset error so a sentence always exists. dispatch({ type: "failed", error: new DatasetError( @@ -71,29 +59,20 @@ const App = () => { }; }, [searchTerm, retryAttempt]); - // Receives the term the search box has settled on rather than every - // keystroke: the debounce lives with the box now, so what arrives here is - // already the term worth issuing a request for. Memoized because the child - // holds on to it, and an empty dependency array is what makes that hold safe. + // Receives the settled term rather than every keystroke. Memoized with an + // empty dependency array, because the child holds on to it. const handleSearchChange = useCallback((term: string) => { setSearchTerm(term); }, []); - // The next keystroke would re-run the search too, but nothing on screen says - // so, which leaves a reader of the error with no way forward. Retrying on a - // timer with backoff was rejected instead: it hides a misconfigured - // deployment behind a spinner and re-downloads several megabytes of city data - // with nobody watching. + // Deliberately manual. A backoff timer would hide a misconfigured deployment + // behind a spinner and re-download the dataset with nobody watching. const handleRetry = useCallback(() => { dispatch({ type: "retry" }); }, []); - // Derived here, during render, rather than at the catch above, and that is - // the whole reason this line is not two lines further up. The catch is inside - // the fetch effect, so reading the catalog there would make the locale a - // dependency of the effect and a reader changing language would re-issue the - // search. Here the catalog is already in scope and the effect's dependencies - // are untouched. + // Derived during render rather than at the catch, which is inside the fetch + // effect: reading the catalog there would make the locale a dependency of it. const errorMessage = error === null ? null : datasetErrorText(error, catalog, tag); diff --git a/src/appState.ts b/src/appState.ts index 2ae8cd0..3ddb979 100644 --- a/src/appState.ts +++ b/src/appState.ts @@ -1,41 +1,23 @@ import type { City } from "./api/getCities"; /** - * Everything the container remembers about the collection it is fetching, in - * one object. - * - * It is one object rather than five values because the fetch effect moves - * several of them together: an attempt raises the loading flag and clears the - * previous failure at the same instant, and separate writes are separate - * chances to tear. + * Everything the container remembers about the collection it is fetching. One + * object, because the effect moves several of these together. */ export interface AppState { readonly cities: City[]; readonly error: Error | null; readonly loading: boolean; - /** - * The loading flag is also true for a refetch that follows an empty result - * set, so on its own it cannot say whether the wait is a download or a - * search. This records the one fact it cannot carry: whether the collection - * has arrived at least once. - */ + /** Whether the collection has arrived once, which loading cannot say. */ readonly datasetReady: boolean; - /** - * The retry control increments this, and the effect lists it as a dependency, - * which is what lets a failed load be run again without a page reload. - */ + /** Incremented by the retry control and listed as an effect dependency. */ readonly retryAttempt: number; } /** - * The five things that happen to a request, named for what happened rather than - * for the fields they write. - * - * Settling is its own action rather than folded into the two outcomes before - * it, because clearing the loading flag from one handler that runs whichever - * way the promise went is precisely what stops a failure leaving a permanent - * spinner. Folding it in would leave that guarantee resting on two branches - * agreeing forever. + * The five things that happen to a request. Settling is its own action, because + * folding it in would leave the no-permanent-spinner guarantee resting on two + * branches agreeing forever. */ export type AppAction = | { readonly type: "attempt" } @@ -44,12 +26,7 @@ export type AppAction = | { readonly type: "settled" } | { readonly type: "retry" }; -/** - * Where the container starts: nothing fetched, nothing failed, nothing in - * flight, and no attempt made. The first attempt is what raises the loading - * flag, so the value here is the state of a container that has just mounted and - * not yet run its effect. - */ +/** Where the container starts: mounted, with its effect not yet run. */ export const INITIAL_APP_STATE: AppState = { cities: [], error: null, @@ -58,12 +35,7 @@ export const INITIAL_APP_STATE: AppState = { retryAttempt: 0, }; -/** - * The only thing that moves the container from one request state to the next. - * - * It is pure and knows nothing about React, so the rules below can be read and - * tested without rendering anything. - */ +/** The only thing that moves the container from one request state to the next. */ export function applyAppAction(state: AppState, action: AppAction): AppState { switch (action.type) { case "attempt": @@ -78,9 +50,8 @@ export function applyAppAction(state: AppState, action: AppAction): AppState { datasetReady: true, }; case "failed": - // The rows already on screen are correct until something replaces them, - // so a failure paints an error beside them rather than emptying the - // table, and the arrival flag stays raised. + // The rows on screen stay correct until something replaces them, so a + // failure paints beside them rather than emptying the table. return { ...state, error: action.error }; case "settled": return { ...state, loading: false }; diff --git a/src/components/DataTable/DataTable.tsx b/src/components/DataTable/DataTable.tsx index 1c0f254..0dd893b 100644 --- a/src/components/DataTable/DataTable.tsx +++ b/src/components/DataTable/DataTable.tsx @@ -10,10 +10,7 @@ import styles from "./DataTable.module.scss"; export type { PaginationLabels }; -/** - * Every string this table and the controls under it render. The entries that - * weave a value take that value and never a word, which would decide grammar. - */ +/** Entries that weave a value take the value, never an assembled word. */ export interface DataTableLabels { /** Shown in place of the whole view until the rows have arrived once. */ readonly loading: string; diff --git a/src/components/DataTable/sortRows.ts b/src/components/DataTable/sortRows.ts index be5ca35..76eb8a5 100644 --- a/src/components/DataTable/sortRows.ts +++ b/src/components/DataTable/sortRows.ts @@ -1,9 +1,6 @@ import type { Column } from "./column"; -/** - * Orders rows by one column. Row identity is a table-level prop, so the - * tiebreak lives here rather than inside a column's comparator. - */ +/** Row identity is a table-level prop, so the tiebreak lives here. */ export function sortRows( rows: readonly T[], column: Column | undefined, @@ -22,10 +19,7 @@ export function sortRows( }); } -/** - * Ascending, never flipped, and never collated: an identity is not a value the - * reader sees, so collating it would give two readers two orders. - */ +/** Never flipped and never collated: an identity is not a visible value. */ export function compareIdentities(aId: string, bId: string): number { if (aId === bId) return 0; return aId < bId ? -1 : 1; diff --git a/src/components/DataTable/tableState.ts b/src/components/DataTable/tableState.ts index c9f54fa..ce67c6a 100644 --- a/src/components/DataTable/tableState.ts +++ b/src/components/DataTable/tableState.ts @@ -15,10 +15,7 @@ export type TableAction = | { readonly type: "pageSize"; readonly pageSize: number } | { readonly type: "query"; readonly query: string }; -/** - * Where a table starts: what the serializer omits and the parser falls back to. - * Typed over no column id, so it is assignable to a state over any id union. - */ +/** Where a table starts. Typed over no id, so it fits any id union. */ export const DEFAULT_TABLE_STATE: TableState = { sortColumnId: null, sortDirection: null, diff --git a/src/components/DataTable/tableStateUrl.ts b/src/components/DataTable/tableStateUrl.ts index 0d88989..0cbbc3f 100644 --- a/src/components/DataTable/tableStateUrl.ts +++ b/src/components/DataTable/tableStateUrl.ts @@ -7,10 +7,7 @@ import { /** One key rather than two, so the invalid pair is unrepresentable. */ const SORT_DESCENDING_PREFIX = "-"; -/** - * One owned parameter. Parsing returns a partial so one key can carry two - * coupled fields; serializing returns null for a defaulted value. - */ +/** Parsing returns a partial; serializing returns null for a default. */ interface UrlParamEntry { readonly key: string; readonly parse: ( diff --git a/src/components/paginate.ts b/src/components/paginate.ts index 40d57c4..0f9d41c 100644 --- a/src/components/paginate.ts +++ b/src/components/paginate.ts @@ -5,10 +5,7 @@ export interface PaginateResult { readonly effectivePage: number; } -/** - * Slices one page out of a collection. The position is an argument and the - * clamp never leaves this function, so it can arrive from anywhere. - */ +/** Slices one page. The position is an argument; the clamp stays here. */ export function paginate( rows: readonly T[], page: number, diff --git a/src/features/CityTable/CityTable.tsx b/src/features/CityTable/CityTable.tsx index e794aa3..9409c14 100644 --- a/src/features/CityTable/CityTable.tsx +++ b/src/features/CityTable/CityTable.tsx @@ -23,11 +23,7 @@ import { } from "./cityColumns"; import styles from "./CityTable.module.scss"; -/** - * How long typing has to pause before the term is committed. The window is the - * one the container applied before this component took the search box over, so - * the move retunes nothing. - */ +/** How long typing has to pause before the term is committed. */ const SEARCH_DEBOUNCE_MS = 150; interface CityTableProps { @@ -38,25 +34,17 @@ interface CityTableProps { readonly loading: boolean; // False until the underlying collection has arrived at least once. readonly datasetReady: boolean; - // The text of the failure rather than the failure itself, which is the shape - // the table below takes: nothing under this component narrows an error, so a - // preserved cause cannot reach a reader by accident. + // The text of the failure rather than the failure itself, so nothing below + // this component narrows an error and a cause cannot reach a reader. readonly errorMessage: string | null; - // Optional so the table stays usable on its own, without a container to - // re-run the request behind it. + // Optional so the table stays usable without a container behind it. readonly onRetry?: () => void; } /** - * The city table: the shared table wired to the city columns, the city copy, - * and the view state that drives them. - * - * Everything this application knows about cities that the table has to render - * is assembled here, which is what leaves the shared component free of it. - * The search box belongs to this component: it holds what is being typed and - * the term that typing settles on, and it reports the settled term upward so - * the container can issue the request behind it. Which fields a term matches is - * still decided at the data layer. + * The shared table wired to the city columns, the city copy and the view state + * that drives them. The search box belongs here: it holds what is being typed + * and the term typing settles on, and reports the settled term upward. */ export function CityTable({ data, @@ -67,45 +55,25 @@ export function CityTable({ onRetry, }: CityTableProps) { // The one place below the header that subscribes to the locale. Everything - // under src/components/ takes its strings as props and never learns that a - // locale exists, which is what keeps the shared table shared. + // under src/components/ takes its strings as props. const { catalog, tag } = useLocale(); - // The deliberate exception to the rule that label objects are built at module - // scope. The table holds this object across renders and two of its entries are - // closures, so its identity has to change when the locale does and must not - // change otherwise. That is exactly what a memo keyed on the catalog and the - // tag gives, and a module-scope constant cannot give it at all. + // The documented exception to module-scope label objects: the table holds + // this across renders and several entries are closures, so its identity has + // to move when the locale does and must not move otherwise. const labels = useMemo(() => buildTableLabels(catalog, tag), [catalog, tag]); - // The search box's own two strings, built from the same catalog on the same - // render so the whole tree changes language at once. Keyed on the catalog - // alone because neither entry weaves a number, so the tag decides nothing - // here and listing it would claim a dependency this does not have. + // Keyed on the catalog alone, because neither entry weaves a number and the + // tag decides nothing here. const searchLabels = useMemo(() => buildSearchLabels(catalog), [catalog]); - // The other documented exception to module-scope construction, and it keys on - // exactly the two values the labels above key on. That is a requirement - // rather than a symmetry: a column array whose identity moved on a render - // where the labels did not would re-sort the whole collection and re-slice - // the page for nothing, which over fifty thousand rows is the most expensive - // thing this component can do by accident. + // Keyed on exactly the two values the labels are, and that is a requirement + // rather than a symmetry: an array identity that moved on its own would + // re-sort fifty thousand rows for nothing. const columns = useMemo(() => buildCityColumns(catalog, tag), [catalog, tag]); // Initialized from whatever the address carries, so the first render is - // already the restored view: a link naming a page never paints the first one - // for a frame on the way there. Reading it here rather than in an effect is - // what buys that, and the initializer is pure, so the development-mode double - // invoke costs one extra parse. - // - // This component holds the view state rather than receiving it the way the - // shared table does, and the reason is measured rather than stylistic. About - // forty renders in the integration suite drive sorting and paging by clicking - // this component, and that suite is the accessibility regression record - // carried forward from an earlier phase. Hoisting the state would put a - // stateful wrapper under every one of them, at which point the suite asserts - // against the wrapper rather than against the application. Four test edits - // against roughly forty is the whole of the argument. + // already the restored view rather than the first page for a frame. const [tableState, setTableState] = useState>( () => ({ ...DEFAULT_TABLE_STATE, @@ -113,22 +81,13 @@ export function CityTable({ }), ); - // What is currently in the box, which is not yet what the table has been - // asked for. Seeded from the committed term, which the initializer above has - // already read out of the address, so a link carrying a term paints that term - // on the first render rather than filling the box in after mount. Declared - // next to the state it settles into so a reader meets the pair together. + // What is in the box, which is not yet what the table was asked for. Seeded + // from the committed term, so a link carrying one paints it on first render. const [searchInput, setSearchInput] = useState(tableState.query); // The only place in this application that writes the address, and it replaces - // rather than pushes, so one Back press leaves the site instead of stepping - // the reader back through positions they never asked to record. - // - // The comparison ahead of the write earns four things at once: a link that is - // already canonical is never rewritten, a parameter stating a default is - // stripped the moment it arrives, a hostile link is canonicalized on arrival, - // and a change driven by a back navigation cannot loop, because by then the - // address already says what the state says. + // rather than pushes. The comparison ahead of the write canonicalizes a + // hostile link on arrival and stops a back navigation looping. // // One address is one view, per resolved locale: the query string carries // the search term, the sort column and direction, the page and the page @@ -141,19 +100,13 @@ export function CityTable({ const next = serializeTableState(tableState, window.location.search); if (next === window.location.search) return; - // An empty query has to be written as the path. The empty string resolves - // to the current address and leaves the stale query exactly where it was, - // which is a write that reports success and changes nothing. The fragment - // rides along in both branches because a relative reference carrying a - // query but no fragment drops the fragment, and a shared link can carry one - // this application never put there. + // An empty query is written as the path: the empty string resolves to the + // current address and leaves the stale query where it was. The fragment + // rides along in both branches or a relative reference would drop it. // - // Guarded because no write to the address is worth the table. Browsers rate - // limit history mutation and throw rather than ignoring the call, and a held - // Enter key on the paging control reaches that ceiling in seconds over a - // collection with thousands of pages. A throw here is a throw in a - // commit-phase effect, so the boundary above would replace the whole view - // with the failure fallback over a link that failed to update. + // Guarded because no write to the address is worth the table. A browser + // that rate limits history mutation throws, and a throw in a commit-phase + // effect would replace the whole view with the failure fallback. try { window.history.replaceState( window.history.state, @@ -167,8 +120,7 @@ export function CityTable({ } }, [tableState]); - // The functional updater form is what keeps these dependency arrays empty, so - // the three callbacks keep one identity for the life of the table. + // The functional updater form is what keeps these dependency arrays empty. const handleSort = useCallback((columnId: CityColumnId) => { setTableState((state) => applyTableAction(state, { type: "sort", columnId }), @@ -185,28 +137,14 @@ export function CityTable({ ); }, []); - // This one carries a dependency where the three above carry none, so its - // identity is an argument rather than a guarantee: the only thing it depends - // on is the parent's callback, and that callback is itself memoized with an - // empty array, so in practice it is as stable as the three. Nothing below it - // inherits that argument, because the debounce reads its callback out of a - // ref rather than closing over it. - // - // Committing is the single point a pause in typing reaches. It moves the view - // state, which returns the reader to the first page because a new term is a - // different set of rows rather than a narrowing of the current one, and it - // reports the term upward so the request behind it is reissued. + // The single point a pause in typing reaches: it moves the view state, which + // returns the reader to the first page, and reports the term upward. Its one + // dependency does not reach the debounce, which reads its callback from a ref. const commitSearch = useCallback( (term: string) => { - // Canonicalized here, once, rather than at each of the three places that - // decide whether two terms are the same view. The search trims before it - // matches and the address trims before it writes, so a term differing - // only in edge whitespace selects the same rows at the same address; - // committing it verbatim is what makes the state disagree with both of - // them, and the disagreement costs the reader their position, strips the - // page from the address, and reissues a request for rows that did not - // change. The box goes on painting what was typed, because what is being - // typed is separate state from what typing settles on. + // Canonicalized once here, because the search trims before it matches + // and the address trims before it writes. The box goes on painting what + // was typed, which is separate state from what typing settles on. const settled = term.trim(); setTableState((state) => @@ -220,21 +158,13 @@ export function CityTable({ const { schedule: scheduleSearchCommit, cancel: cancelSearchCommit } = useDebouncedCallback(commitSearch, SEARCH_DEBOUNCE_MS); - // A history entry this application did not create can still carry a query it - // owns, so a traversal re-reads the whole view from the address and applies - // it in one write. Attached and detached symmetrically rather than assigned - // onto the window, so a second listener cannot silently replace this one. - // - // Declared after the scheduler rather than beside the write above, because it - // has to be able to cancel a commit the scheduler is still holding, and a - // dependency array is read during the render that declares it. + // A traversal re-reads the whole view from the address and applies it in one + // write. Declared after the scheduler, because it has to cancel a commit the + // scheduler is still holding. useEffect(() => { const handlePopState = () => { - // A traversal landing inside the debounce window would otherwise let the - // term the reader typed a moment ago land on top of the view they just - // navigated back to: the box would show the restored term while the rows, - // the position, and the address all carried the typed one. The keystrokes - // belong to the view the reader has left, so the commit goes with it. + // The keystrokes belong to the view the reader has left, so a commit + // still pending goes with it rather than landing on the restored view. cancelSearchCommit(); const restored: TableState = { @@ -244,20 +174,13 @@ export function CityTable({ setTableState((state) => ({ ...restored, - // A restored sort is still a first render, and announcing a sort to - // someone who has just opened a link announces something that did not - // just happen. Carrying the flag across means a traversal after a real - // sort still announces, while a cold one stays silent. It is the one - // field carried over, because it is the one field the address does not - // and will not hold. + // The one field carried over, because it is the one the address does + // not hold: a restored sort is still a first render and stays silent. hasSorted: state.hasSorted, })); - // The box and the request behind it both follow the term the traversal - // landed on. Reporting it upward rather than letting the container read - // the address for itself keeps the single writer single and the reader - // count at two, and it is why this handler depends on the parent's - // callback where the three above depend on nothing. + // Reported upward rather than read from the address by the container, + // which is what keeps the reader count at two. setSearchInput(restored.query); onSearchChange(restored.query); }; @@ -268,8 +191,7 @@ export function CityTable({ }; }, [onSearchChange, cancelSearchCommit]); - // The box repaints on every keystroke while the commit waits for the pause, - // so typing stays responsive and the table is asked once for what was typed. + // The box repaints on every keystroke while the commit waits for the pause. const handleSearchChange = useCallback( (term: string) => { setSearchInput(term); diff --git a/src/features/CityTable/cityColumns.ts b/src/features/CityTable/cityColumns.ts index 014fa2f..af23c9e 100644 --- a/src/features/CityTable/cityColumns.ts +++ b/src/features/CityTable/cityColumns.ts @@ -6,27 +6,11 @@ import { collatorFor, numberFormatFor } from "../../i18n/format"; import { resolveLocale } from "../../i18n/resolveLocale"; /** - * The columns the city table shows, in the order it shows them, built for one - * resolved locale. - * - * A builder rather than the module-scope array this used to be, because both - * halves of a column now follow the reader: the label comes out of the catalog, - * and the population cell is grouped by the tag's own rule rather than by - * whatever the machine running the code prefers. The collator goes in here too, - * fused into the default comparator at construction the way the accessor - * already is, which is what leaves the sort module and the table's own props - * untouched by any of this. - * - * The population formatting lives here because it is a fact about this column - * of this dataset, and the table body that renders it knows nothing about - * either. It goes through the cached formatter rather than the value's own - * per-call helper, which builds a formatter on every call and so builds one per - * rendered cell per render. - * - * The array this returns is a new identity every call, which is the whole point - * and also the hazard: the caller has to build it in a memo keyed on the - * catalog and the tag, or the sort and page memos downstream re-run on every - * render. + * The columns the city table shows, built for one resolved locale: both the + * label and the grouped population cell follow the reader, and the collator is + * fused into the default comparator here. A new identity every call, which is + * the hazard: the caller memoizes on the catalog and the tag, or the sort and + * page memos downstream re-run on every render. */ export function buildCityColumns(catalog: Catalog, tag: string) { const col = columns(collatorFor(tag)); @@ -47,41 +31,22 @@ export function buildCityColumns(catalog: Catalog, tag: string) { /** * One build at module scope, for the id union and the closed set below and for - * nothing else. Neither of those is a fact about a locale: which columns exist - * is the same in every language, and only what they are called moves. Deriving - * them from a build rather than declaring them beside it is what keeps a column - * that is added, renamed or removed from leaving a stale entry here. + * nothing else. Which columns exist is the same in every language; only what + * they are called moves. */ const BASE_COLUMNS = buildCityColumns(en, resolveLocale("en", []).tag); /** The literal union of the ids above, formed with no assertion anywhere. */ export type CityColumnId = (typeof BASE_COLUMNS)[number]["id"]; -/** - * The closed set a sort id restored from an address is checked for membership - * in. Derived from the columns rather than written out beside them, so a column - * that is added, renamed, or removed cannot leave a stale entry behind here. - */ +/** The closed set a restored sort id is checked against, derived not listed. */ export const CITY_COLUMN_IDS: readonly CityColumnId[] = BASE_COLUMNS.map( (column) => column.id, ); -/** - * The width every city id is padded to before it is handed over as a row - * identity. Ten because the dataset's ids are geoname ids, which the generator - * emits at ten digits, and the two rows the parse boundary numbers itself. A - * dataset whose ids outgrow this pads to no effect and the identities start - * ordering as text again, which is a visible reordering rather than a crash, so - * the constant is stated here rather than inlined. - */ +/** Ten, because the dataset's geoname ids are ten digits. */ const ID_WIDTH = 10; -/** - * A row's identity, as text, because that is what the table's tiebreak - * compares. Padded so identities that order as text order as the numbers they - * are: unpadded, "2" follows "1934976309" and the two lowest ids land at the - * end of every group of rows whose sorted column values are equal. City ids are - * unique by construction at the parse boundary, and padding preserves that. - */ +/** Text, because the tiebreak compares text. Padded so "2" precedes "19". */ export const cityRowId = (city: City) => String(city.id).padStart(ID_WIDTH, "0"); diff --git a/src/features/CityTable/cityLabels.ts b/src/features/CityTable/cityLabels.ts index 229625d..bbbe3c5 100644 --- a/src/features/CityTable/cityLabels.ts +++ b/src/features/CityTable/cityLabels.ts @@ -3,17 +3,9 @@ import type { SearchInputLabels } from "../../components/SearchInput"; import type { Catalog } from "../../i18n/catalogs/en"; /** - * The catalog, narrowed to the object the shared table takes. - * - * A catalog is a flat set of keys; the table's labels are a shape, with the page - * controls' own strings nested where the table hands them on. This is where the - * one becomes the other, and it is the only place the two vocabularies meet. - * - * The locale-sensitive entries take the resolved language tag ahead of the - * arguments the table supplies, and closing that tag in here is what keeps the - * table's own contract at the arity it has always had. An entry that needs no - * tag is passed through by reference rather than wrapped in an arrow that would - * only forward it, so the closure count stays at what the seam actually needs. + * The catalog, narrowed to the object the shared table takes: a flat set of + * keys becomes a shape. Closing the tag into the locale-sensitive entries here + * is what keeps the table's contract at the arity it has always had. */ export function buildTableLabels( catalog: Catalog, @@ -44,17 +36,7 @@ export function buildTableLabels( }; } -/** - * The catalog, narrowed to the two strings the search box shows. - * - * Its own builder rather than a third field on the object above, because the box - * is a sibling of the table rather than part of it: the table never sees these - * two strings and has no business carrying them. - * - * No tag, and the absence is the point. Neither entry weaves a number or a - * plural, so there is nothing here for a formatter to do, and taking a parameter - * that changes nothing would put a second key on the memo that reads it. - */ +/** The search box's own two strings. No tag: neither weaves a number. */ export function buildSearchLabels(catalog: Catalog): SearchInputLabels { return { name: catalog.searchName, diff --git a/src/features/CityTable/index.ts b/src/features/CityTable/index.ts index 40ad9d4..911a11c 100644 --- a/src/features/CityTable/index.ts +++ b/src/features/CityTable/index.ts @@ -1,7 +1,5 @@ export { CityTable } from "./CityTable"; -// The container above this feature needs the restored search term on its very -// first render, and this feature is what puts the term in the address. Passing -// the reader back out here keeps that dependency pointed at the feature rather -// than reaching past it into the shared table. +// The container needs the restored term on its first render, and this feature +// owns the address, so the reader is passed back out here. export { parseSearchTerm } from "../../components/DataTable/tableStateUrl"; diff --git a/src/features/Footer/Footer.tsx b/src/features/Footer/Footer.tsx index c5b1292..68e093f 100644 --- a/src/features/Footer/Footer.tsx +++ b/src/features/Footer/Footer.tsx @@ -1,16 +1,9 @@ import { useLocale } from "../../hooks/useLocale"; import styles from "./Footer.module.scss"; -// CC BY 4.0 asks for four things: credit the creator, link the source, link the -// licence, and say whether the work was changed. The catalog sentence is the -// whole of that obligation in every language, which is why it is unconditional -// and why Footer.test.tsx asserts it in more than one. Both modifications named -// there are corroborated by src/data/worldcities/license.txt and by what -// scripts/generate-cities.mjs does. -// -// The two names and the two addresses stay untranslated and stay here. They are -// identifiers rather than copy: the source calls itself this, and the licence -// identifier is what both a machine and a lawyer read. +// CC BY 4.0 asks for credit, a source link, a license link and a record of what +// changed. The catalog sentence carries all four in every language, which is why +// it is unconditional. The two names stay untranslated: they are identifiers. const SOURCE_NAME = "simplemaps.com World Cities"; const LICENSE_NAME = "CC BY 4.0"; @@ -19,15 +12,9 @@ const LINK_URLS: Readonly> = { [LICENSE_NAME]: "https://creativecommons.org/licenses/by/4.0/", }; -// Splits the sentence around the two identifiers, keeping each as a part of its -// own so it can be rendered as a link wherever the sentence put it. One catalog -// entry rather than the three fragments this would otherwise need: three would -// hold every language to English word order, and the pseudo-locale exists partly -// to make a sentence assembled out of several entries visible as several -// bracketed units. -// -// The only regular expression metacharacter in either name is a dot, which -// matches the dot it stands for. +// Splits the sentence around the two identifiers, so each can be a link +// wherever the sentence put it. One catalog entry rather than three fragments, +// which would hold every language to English word order. const EMBEDDED_NAMES = new RegExp(`(${SOURCE_NAME}|${LICENSE_NAME})`); export function Footer() { @@ -42,10 +29,8 @@ export function Footer() { .map((part) => { const href = LINK_URLS[part]; - // The part itself is the key. The split alternates run of text - // with identifier, so no two siblings carry the same string, and - // the position a part sits at is exactly what a translation is - // free to move. + // The part itself is the key: the split alternates text with + // identifier, and position is what a translation may move. return href === undefined ? ( part ) : ( diff --git a/src/features/Header/LocaleControl.tsx b/src/features/Header/LocaleControl.tsx index 47dcb3a..faa999b 100644 --- a/src/features/Header/LocaleControl.tsx +++ b/src/features/Header/LocaleControl.tsx @@ -6,27 +6,15 @@ import { CATALOG_IDS } from "../../i18n/resolveLocale"; import styles from "./LocaleControl.module.scss"; /** - * The language picker: following the machine, then one option per catalog that - * ships, each named in its own language. - * - * A native select rather than the segmented control its neighbour uses. Five - * options do not fit a row of buttons across a header bar, the page-size control - * in the table below is already a native select, and the native control brings - * keyboard handling, mobile behaviour and accessibility with nothing written by - * hand. - * - * The option list is built from the shipped ids rather than written out, so a - * catalog added later appears here without this file being edited. - * - * Its own two strings come from the catalog like every other word on the page. - * The autonyms below do not, and must not: a reader who cannot read the - * interface in front of them still has to find their own language in the list. + * The language picker: follow the machine, then one option per shipped catalog, + * each named in its own language. A native select rather than the segmented + * control beside it, because five options do not fit a row of buttons and the + * native control brings its keyboard and mobile behavior with it. */ export function LocaleControl() { const { catalog, choice, setChoice } = useLocale(); - // Document-global, so a constant here would give a second mounted control the - // same id and bind both labels to the first select. + // Document-global, so a constant would bind two labels to one select. const selectId = useId(); return ( diff --git a/src/features/Header/ThemeControl.tsx b/src/features/Header/ThemeControl.tsx index 00b3355..5bd1d2a 100644 --- a/src/features/Header/ThemeControl.tsx +++ b/src/features/Header/ThemeControl.tsx @@ -7,41 +7,28 @@ import type { ThemeChoice } from "../../theme/resolveTheme"; import styles from "./ThemeControl.module.scss"; /** - * The theme picker: three states, all visible at once, so choosing the operating - * system is as explicit as choosing a theme rather than being the absence of a - * choice. + * The theme picker: three states, all visible at once, so choosing the system + * is as explicit as choosing a theme. * - * a11y: every keyboard behaviour the radiogroup pattern calls for comes from the - * native inputs. Three radios sharing a name give arrow-key movement, wrap - * around, and one tab stop for the group, entered at the checked option. Nothing - * here handles a key, and nothing here should: the previous phase deliberately - * removed exactly this class of hand-written key handling from the sort header. - * - * This diverges from the pattern's non-native form in one place, deliberately: - * the checked state is left entirely to the inputs and no ARIA attribute - * restates it. Native radios already expose it through the property, and an - * attribute layered on top can only ever go stale against it. The prose here - * avoids naming that attribute so a search for it finds live code rather than a - * mention of it. + * a11y: every keyboard behavior the radiogroup pattern calls for comes from the + * native inputs. Nothing here handles a key, and nothing here should. The + * checked state is left to the inputs, because an ARIA attribute layered on top + * of the property can only ever go stale against it. */ export function ThemeControl() { const { choice, setChoice } = useTheme(); const { catalog } = useLocale(); - // The three states are offered in the order the theme vocabulary declares - // them, and each is named from the catalog. A record rather than a label - // beside each value, so the option list stays the one definition of what the - // states are and this is only how they are spelled. + // A record rather than a label beside each value, so the option list stays + // the one definition of what the states are. const names: Readonly> = { light: catalog.themeLight, dark: catalog.themeDark, system: catalog.themeSystem, }; - // The id and the radio name are both document-global, so writing either as a - // constant makes a second mounted control produce duplicate ids and put all - // six radios in one group, where every label binds to the first matching - // input. Generated per instance, the two controls cannot reach each other. + // Document-global, so a constant would put a second mounted control's radios + // in this group and bind its labels to these inputs. const groupName = useId(); return ( diff --git a/src/features/RootLayout/ErrorBoundary.tsx b/src/features/RootLayout/ErrorBoundary.tsx index 40ca136..da3c68c 100644 --- a/src/features/RootLayout/ErrorBoundary.tsx +++ b/src/features/RootLayout/ErrorBoundary.tsx @@ -5,17 +5,9 @@ import styles from "./RootLayout.module.css"; import type { ReactNode } from "react"; /** - * The fallback's two strings. - * - * They arrive as props rather than being read from the catalog here, and the - * reason is the class: this is the only render-fallback mechanism React offers - * and it can only be a class component, so it cannot call a hook. Its parent is - * the locale subscriber and hands the copy down. - * - * The fallback shows this authored copy and nothing else. A render-time throw's - * own message is whatever the engine produced, so it would be noise on screen - * and mild information disclosure, and the surrounding sentence would have to - * work as a frame around an arbitrary string. + * The fallback's two strings, as props because a class cannot call a hook. The + * fallback shows this authored copy and never the throw's own message, which is + * engine text and mild information disclosure. */ export interface ErrorBoundaryLabels { readonly message: string; @@ -31,10 +23,8 @@ interface ErrorBoundaryState { hasError: boolean; } -// The only render-fallback mechanism React offers. The root error callbacks -// added in React 19 report a caught error but render nothing, so they are not -// an alternative to this. Boundaries also cannot catch a promise rejection, -// which is why the table keeps its own inline error region beside this one. +// The only render-fallback mechanism React offers. It cannot catch a promise +// rejection, which is why the table keeps its own inline error region. export class ErrorBoundary extends Component< ErrorBoundaryProps, ErrorBoundaryState @@ -47,9 +37,8 @@ export class ErrorBoundary extends Component< // The reporting lifecycle method is omitted; a client-only bundle has nowhere to send a report. - // Resetting the boundary's own state re-renders the children. A document - // reload would throw away the fetched dataset and re-download roughly three - // megabytes to recover from what is most likely a render-local fault. + // Resetting state re-renders the children. A reload would re-download the + // whole dataset to recover from a probably render-local fault. handleReset = () => { this.setState({ hasError: false }); }; diff --git a/src/features/RootLayout/RootLayout.tsx b/src/features/RootLayout/RootLayout.tsx index f148430..5cb4b6d 100644 --- a/src/features/RootLayout/RootLayout.tsx +++ b/src/features/RootLayout/RootLayout.tsx @@ -11,10 +11,8 @@ interface RootLayoutProps { } export function RootLayout({ children }: RootLayoutProps) { - // The layout subscribes to the locale on the boundary's behalf. A class - // component cannot call a hook, and the boundary below has to be a class - // because that is the only render-fallback mechanism React offers, so its two - // strings are read here and handed down. + // Subscribed here on the boundary's behalf: a class cannot call a hook, and + // the boundary has to be a class. const { catalog } = useLocale(); return ( diff --git a/src/hooks/useDebouncedCallback.ts b/src/hooks/useDebouncedCallback.ts index d3abef1..89be848 100644 --- a/src/hooks/useDebouncedCallback.ts +++ b/src/hooks/useDebouncedCallback.ts @@ -6,10 +6,7 @@ export interface DebouncedCallback { readonly cancel: () => void; } -/** - * Debounces a call rather than a value, and reads the callback from a ref when - * the timer fires, which is what makes an inline arrow safe to pass. - */ +/** Debounces a call, reading the callback from a ref when the timer fires. */ export function useDebouncedCallback( callback: (...args: A) => void, delay: number, diff --git a/src/hooks/useLocale.ts b/src/hooks/useLocale.ts index b1ae607..024f183 100644 --- a/src/hooks/useLocale.ts +++ b/src/hooks/useLocale.ts @@ -8,10 +8,7 @@ import { subscribeLocale, } from "../i18n/localeStore"; -/** - * Owns the reader's locale for one component. Many instances by construction: - * it holds no state, so every subscriber resolves the identical value. - */ +/** Many instances by construction, because it holds no state of its own. */ export function useLocale() { const locale = useSyncExternalStore(subscribeLocale, getLocaleSnapshot); const choice = useSyncExternalStore(subscribeLocale, getChoiceSnapshot); diff --git a/src/hooks/useTheme.ts b/src/hooks/useTheme.ts index de4fbe0..6e817bd 100644 --- a/src/hooks/useTheme.ts +++ b/src/hooks/useTheme.ts @@ -8,10 +8,7 @@ import { } from "../theme/resolveTheme"; import type { ThemeChoice } from "../theme/resolveTheme"; -/** - * Reads the stored choice; anything but the two explicit words is absent. The - * property access is what throws when site data is blocked, hence the catch. - */ +/** Anything but the two explicit words is absent. The access can throw. */ function readStoredChoice(): ThemeChoice { try { const stored = localStorage.getItem(THEME_STORAGE_KEY); @@ -43,10 +40,7 @@ function readPrefersDark(): boolean { return window.matchMedia(PREFERS_DARK_QUERY).matches; } -/** - * Subscribes to the system preference. Through useSyncExternalStore rather than - * an effect, which would leave a window for the preference to move. - */ +/** Through useSyncExternalStore, not an effect, which leaves a window. */ function subscribePrefersDark(onStoreChange: () => void): () => void { if (!supportsMediaQueries()) { return () => {}; diff --git a/src/i18n/format.ts b/src/i18n/format.ts index 35ef8ac..6c90fe6 100644 --- a/src/i18n/format.ts +++ b/src/i18n/format.ts @@ -41,10 +41,7 @@ export function pluralRulesFor(tag: string): Intl.PluralRules { return cached(pluralRules, tag, (forTag) => new Intl.PluralRules(forTag)); } -/** - * Picks a noun's form over the categories the tag reports. A - * singular-or-other pair is wrong in half the shipped catalogs. - */ +/** Over the categories the tag reports, not a singular-or-other pair. */ export function selectPlural( tag: string, count: number, From 92508b63089132cf67eebe97c7007fc562771dce Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:22 -0700 Subject: [PATCH 14/20] fix(lint): hold the rules the config claimed to hold The rules key beside the React recommended set replaced it rather than merging with it, so only one of its rules was on. The colour rule saw no functional notation and the unit rule saw no longhand of the ten properties it named. A toolchain case now asserts the React set is active, which is the positive claim a lint config cannot make about itself. --- .stylelintrc.json | 30 +++++++++++++++-------- eslint.config.js | 20 ++++++++++++---- src/toolchain.test.ts | 56 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 14 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index 2d71844..c950eaf 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -76,20 +76,30 @@ ], "declaration-property-unit-allowed-list": [ { - "margin": ["rem"], - "margin-block": ["rem"], - "margin-inline": ["rem"], - "padding": ["rem"], - "padding-block": ["rem"], - "padding-inline": ["rem"], - "gap": ["rem"], - "row-gap": ["rem"], - "column-gap": ["rem"], - "font-size": ["rem"] + "/^(margin|padding)(-(block|inline))?(-(start|end))?$/": ["rem"], + "/^(margin|padding)-(top|bottom|left|right)$/": ["rem"], + "/^(gap|row-gap|column-gap|font-size)$/": ["rem"] }, { "message": "Spacing and type are authored in rem through a token, so the layout follows the reader's browser font-size setting: reach for var(--space-*) or var(--font-size-*)" } + ], + "function-disallowed-list": [ + [ + "rgb", + "rgba", + "hsl", + "hsla", + "hwb", + "lab", + "lch", + "oklab", + "oklch", + "color-mix" + ], + { + "message": "Colour in a component stylesheet arrives through var(--color-*) and nowhere else; a functional notation is as fixed as a hex and flips with no theme" + } ] } }, diff --git a/eslint.config.js b/eslint.config.js index de1fa74..4797b5c 100755 --- a/eslint.config.js +++ b/eslint.config.js @@ -35,7 +35,7 @@ const TEST_SCAFFOLDING_IMPORT = { // silently unrestrict it for every file that block matches. const READER_FACING_ATTRIBUTE = { selector: - 'JSXAttribute[name.name=/^(aria-label|title|placeholder|alt)$/][value.type="Literal"]', + 'JSXAttribute[name.name=/^(aria-label|title|placeholder|alt)$/]:matches([value.type="Literal"], [value.expression.type="Literal"], [value.expression.type="TemplateLiteral"])', message: "src/components/ renders any collection for any reader, so a string here is a claim about which reader. Add the entry to the catalogs and read it off DataTableLabels, PaginationLabels or SearchInputLabels.", }; @@ -78,7 +78,9 @@ const DIRECTION_BRANCH = [ }, { selector: - "IfStatement[alternate] ReturnStatement[argument.type=/^JSX(Element|Fragment)$/]", + "IfStatement" + + ":matches([consequent.argument.type=/^JSX(Element|Fragment)$/], [consequent.body.0.argument.type=/^JSX(Element|Fragment)$/])" + + ":matches([alternate.argument.type=/^JSX(Element|Fragment)$/], [alternate.body.0.argument.type=/^JSX(Element|Fragment)$/])", message: "Two glyph components returned from two branches are a prop, a branch and a coverage line for what one transform: scaleX(-1) under an attribute selector already does. Mirror in the stylesheet.", }, @@ -90,7 +92,15 @@ export default defineConfig([ // emitted bundle, a coverage report, and the page resources the visual spec // archives under test-results/. { - ignores: ["dist/", "coverage/", "test-results/", "playwright-report/"], + ignores: [ + "dist/", + "coverage/", + "test-results/", + "playwright-report/", + "junit/", + "src/__screenshots__/", + ".vitest-attachments/", + ], }, { files: ["**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"], @@ -123,6 +133,9 @@ export default defineConfig([ }, }, rules: { + // Spread again: the key above replaces the recommended rules wholesale + // rather than merging with them, which silently disabled all 22. + ...react.configs.flat.recommended.rules, "react/react-in-jsx-scope": "off", }, }, @@ -187,7 +200,6 @@ export default defineConfig([ "src/i18n/format.ts", "src/**/*.test.{ts,tsx}", "src/**/*.test-d.ts", - "src/test/**", ], rules: { "no-restricted-syntax": ["error", ...LOCALE_CALL_SITE], diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index 95e4037..d5d9356 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -1776,3 +1776,59 @@ describe("toolchain baseline", () => { ).toEqual(["Intl.Collator", "Intl.NumberFormat", "Intl.PluralRules"]); }); }); + +describe("the plugin rule sets the lint gate claims to run", () => { + // A flat-config block that spreads a shared config and then declares its own + // rules key replaces that config's rules wholesale rather than merging with + // them. The gate stays green, because the rules are simply absent. Same + // per-object replacement hazard eslint.config.js records for + // no-restricted-imports, one level up, and no disallow rule can state the + // positive claim that a rule set is still on. + // + // The one rule turned off on purpose: the new JSX transform needs no import + // in scope. Listed here so a second name joining it has to be deliberate. + const DELIBERATELY_OFF = ["react/react-in-jsx-scope"]; + + const severityOf = (entry: unknown): unknown => + Array.isArray(entry) ? entry[0] : entry; + + const isOff = (entry: unknown): boolean => { + const severity = severityOf(entry); + return severity === 0 || severity === "off" || severity === undefined; + }; + + it("has every rule of the React recommended set active", async () => { + const { ESLint } = await import("eslint"); + const react = (await import("eslint-plugin-react")).default; + + const resolved: unknown = await new ESLint({ + cwd: projectRoot, + }).calculateConfigForFile( + join(projectRoot, "src/components/DataTable/TableHead.tsx"), + ); + const active = + (resolved as { rules?: Record }).rules ?? {}; + + const recommended = required( + react.configs.flat.recommended, + "the React recommended flat config", + ).rules; + + // The plugin ships a few of its own recommended entries at severity 0, so + // the claim is over the ones it actually enables. + const enabled = Object.entries(recommended ?? {}) + .filter(([, entry]) => !isOff(entry)) + .map(([name]) => name) + .filter((name) => !DELIBERATELY_OFF.includes(name)); + + expect( + enabled.length, + "the React recommended set is empty", + ).toBeGreaterThan(10); + + expect( + enabled.filter((name) => isOff(active[name])), + "rules of the React recommended set are not on", + ).toEqual([]); + }); +}); From ee59101dd675d947d9b70de2ef5f9209d619ef09 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:28 -0700 Subject: [PATCH 15/20] docs: restore the invariants the trim removed Five comments kept a conclusion without the fact behind it: the unconditional cache clear, the re-read in subscribeLocale, the pseudo-locale's primary subtag, the reachability of the mixed comparison arms, and why the error codes are a tuple. --- src/components/compareRows.ts | 5 ++++- src/data/worldcities/cities.ts | 10 ++++++++-- src/i18n/localeStore.ts | 6 +++++- src/i18n/resolveLocale.ts | 6 ++++-- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/components/compareRows.ts b/src/components/compareRows.ts index 58fa4ab..bf95d64 100644 --- a/src/components/compareRows.ts +++ b/src/components/compareRows.ts @@ -8,7 +8,10 @@ function isBlank(value: unknown): boolean { /** Grouping by type keeps the comparison transitive over mixed rows. */ const TYPE_RANK = { number: 0, string: 1, other: 2 } as const; -/** Places a value in the ordering above. The rows reach here already widened. */ +/** + * Places a value in the ordering above. The parse boundary types every City + * field, so only a row type other than City reaches the mixed arms. + */ function rank(value: unknown): number { if (typeof value === "number") return TYPE_RANK.number; if (typeof value === "string") return TYPE_RANK.string; diff --git a/src/data/worldcities/cities.ts b/src/data/worldcities/cities.ts index ca65fff..6dae5c8 100644 --- a/src/data/worldcities/cities.ts +++ b/src/data/worldcities/cities.ts @@ -34,7 +34,12 @@ export interface City { population: number; } -/** The ways loading can fail. Codes, because the messages below are English. */ +/** + * The ways loading can fail. Codes, because the messages below are English. + * Eight are thrown here; "unexpected" is App's fallback for a rejection that + * carries no Error. A tuple, so the catalog test walks the set rather than + * restating it. + */ export const DATASET_ERROR_CODES = [ "notAnObject", "missingRows", @@ -213,7 +218,8 @@ export function loadCities(): Promise { .then(parseCities); // Attached at store time: any delay leaves a window in which a retry - // re-awaits the already-rejected promise. + // re-awaits the already-rejected promise. Unconditional is safe, because a + // new entry is only stored after this handler has cleared the old one. pending.catch(() => { cached = undefined; }); diff --git a/src/i18n/localeStore.ts b/src/i18n/localeStore.ts index 47e712f..732ef10 100644 --- a/src/i18n/localeStore.ts +++ b/src/i18n/localeStore.ts @@ -43,7 +43,11 @@ function handleStorage(event: StorageEvent): void { } } -/** The listeners come and go with the first and last subscriber. */ +/** + * The listeners come and go with the first and last subscriber, so the choice + * is re-read here: nothing was listening between the last unsubscribe and this + * call, and a cross-tab write in that window went unseen. + */ export function subscribeLocale(onStoreChange: () => void): () => void { if (subscribers.size === 0) { choice = readStoredChoice(); diff --git a/src/i18n/resolveLocale.ts b/src/i18n/resolveLocale.ts index ab4c58f..2ffe348 100644 --- a/src/i18n/resolveLocale.ts +++ b/src/i18n/resolveLocale.ts @@ -9,8 +9,10 @@ export const CATALOG_IDS = ["en", "es", "fr", "ar-XB"] as const; export type CatalogId = (typeof CATALOG_IDS)[number]; /** - * The catalogs a preference list may select. The pseudo-locale is excluded, so - * it stays reachable the only way it should be, by being chosen. + * The catalogs a preference list may select. The pseudo-locale's primary + * subtag is ar, and negotiation matches on that, so an unfiltered walk would + * serve bracketed English to a reader who wants Arabic. Excluding it here + * leaves it reachable the only way it should be, by being chosen. */ export const NEGOTIABLE_CATALOG_IDS = ["en", "es", "fr"] as const; From 29a048aa6f707c2daf7bd39d7d43f98efd4e2429 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:51:38 -0700 Subject: [PATCH 16/20] fix: close the sharp edges the review found The footer indexed an object literal with a slice of catalog copy, so a part named for an Object.prototype member rendered as a link; it is a Map now. The ring rule read a bare 0 channel inside a colour function as a suppression, and the visually-hidden exemption nulled the unit rule for a whole file to excuse one negative margin. --- .stylelintrc.json | 23 +++++++++++++---------- src/features/Footer/Footer.tsx | 12 +++++++----- src/i18n/catalogs/pseudo.ts | 2 +- src/i18n/localeStore.ts | 3 ++- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index c950eaf..b136c15 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -37,15 +37,9 @@ "outline": [ "/(?> = { - [SOURCE_NAME]: "https://simplemaps.com/data/world-cities", - [LICENSE_NAME]: "https://creativecommons.org/licenses/by/4.0/", -}; +// A Map rather than an object literal: the key is a slice of catalog copy, and +// an object answers for every Object.prototype member as well as its own. +const LINK_URLS = new Map([ + [SOURCE_NAME, "https://simplemaps.com/data/world-cities"], + [LICENSE_NAME, "https://creativecommons.org/licenses/by/4.0/"], +]); // Splits the sentence around the two identifiers, so each can be a link // wherever the sentence put it. One catalog entry rather than three fragments, @@ -27,7 +29,7 @@ export function Footer() { .attribution(SOURCE_NAME, LICENSE_NAME) .split(EMBEDDED_NAMES) .map((part) => { - const href = LINK_URLS[part]; + const href = LINK_URLS.get(part); // The part itself is the key: the split alternates text with // identifier, and position is what a translation may move. diff --git a/src/i18n/catalogs/pseudo.ts b/src/i18n/catalogs/pseudo.ts index 638650d..25a146c 100644 --- a/src/i18n/catalogs/pseudo.ts +++ b/src/i18n/catalogs/pseudo.ts @@ -5,7 +5,7 @@ import { type SortedDirection, } from "./en"; -/** The pseudo-locale, so direction and truncation have something to prove. */ +// The pseudo-locale, so direction and truncation have something to prove. /** An isolate, not a mark: a mark cannot bound a run, only start one. */ const FIRST_STRONG_ISOLATE = "\u2066"; diff --git a/src/i18n/localeStore.ts b/src/i18n/localeStore.ts index 732ef10..b807246 100644 --- a/src/i18n/localeStore.ts +++ b/src/i18n/localeStore.ts @@ -7,7 +7,8 @@ import { type ResolvedLocale, } from "./resolveLocale"; -/** Owned once for the document, which is what makes several callers agree. */ +// The chosen locale, owned once for the document, which is what makes several +// callers agree rather than each resolving one of its own. /** Anything not a shipped id is absent. The access throws when blocked. */ function readStoredChoice(): LocaleChoice { From 3f211dc9af5934cdf933ce155f788348bc13268f Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:14:37 -0700 Subject: [PATCH 17/20] fix(lint): stop the restored rules over-reaching The colour-function rule fired on the token file, which is the one file allowed to declare a colour, and the ring rule read a bare 0 channel inside rgb() as a suppression. The attribute rule flagged a template that carries an interpolation and so composes at runtime. Adds color() to the colour list, and holds the guard's sample file to one that exists. --- .stylelintrc.json | 6 ++++-- eslint.config.js | 11 +++++++++-- src/styles/_visually-hidden.scss | 9 +++++---- src/toolchain.test.ts | 9 ++++++--- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/.stylelintrc.json b/.stylelintrc.json index b136c15..4be2792 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -89,7 +89,8 @@ "lch", "oklab", "oklch", - "color-mix" + "color-mix", + "color" ], { "message": "Colour in a component stylesheet arrives through var(--color-*) and nowhere else; a functional notation is as fixed as a hex and flips with no theme" @@ -100,7 +101,8 @@ { "files": ["src/index.css"], "rules": { - "color-no-hex": null + "color-no-hex": null, + "function-disallowed-list": null } }, { diff --git a/eslint.config.js b/eslint.config.js index 4797b5c..d4bfc49 100755 --- a/eslint.config.js +++ b/eslint.config.js @@ -35,7 +35,7 @@ const TEST_SCAFFOLDING_IMPORT = { // silently unrestrict it for every file that block matches. const READER_FACING_ATTRIBUTE = { selector: - 'JSXAttribute[name.name=/^(aria-label|title|placeholder|alt)$/]:matches([value.type="Literal"], [value.expression.type="Literal"], [value.expression.type="TemplateLiteral"])', + 'JSXAttribute[name.name=/^(aria-label|title|placeholder|alt)$/]:matches([value.type="Literal"], [value.expression.type="Literal"], [value.expression.type="TemplateLiteral"][value.expression.expressions.length=0])', message: "src/components/ renders any collection for any reader, so a string here is a claim about which reader. Add the entry to the catalogs and read it off DataTableLabels, PaginationLabels or SearchInputLabels.", }; @@ -77,6 +77,11 @@ const DIRECTION_BRANCH = [ "Two glyph components chosen on a condition are a prop, a branch and a coverage line for what one transform: scaleX(-1) under an attribute selector already does. Mirror in the stylesheet.", }, { + // Both sides, by shape: esquery cannot scope :has() to a named property, + // so an else-if chain and a return that is not first in its block are out + // of reach. Recorded in GUARD-DISPOSITION.md rather than answered with a + // looser selector, because the guarded render this used to flag is the + // false positive that gets a rule deleted. selector: "IfStatement" + ":matches([consequent.argument.type=/^JSX(Element|Fragment)$/], [consequent.body.0.argument.type=/^JSX(Element|Fragment)$/])" + @@ -193,7 +198,9 @@ export default defineConfig([ // test globs are exempt because a test asserting a formatted string has to // compute its expectation through the platform rather than type it: the French // group separator is a narrow no-break space, and a typed literal fails on a - // difference no terminal renders. So the ban is on shipped call sites. + // difference no terminal renders. src/test/ is not exempt: it holds executing + // helpers rather than assertions, and the sweep it drives is shipped code's + // reach. { files: ["src/**/*.{ts,tsx}"], ignores: [ diff --git a/src/styles/_visually-hidden.scss b/src/styles/_visually-hidden.scss index fe4150f..7013e1a 100644 --- a/src/styles/_visually-hidden.scss +++ b/src/styles/_visually-hidden.scss @@ -9,10 +9,11 @@ height: 1px; padding: 0; - // The one px length on a spacing property in the tree, and the reason this - // file nulls declaration-property-unit-allowed-list in .stylelintrc.json. A - // unit allowed-list cannot say "a pixel or two", which is what the negative - // margin is: the classic off-screen clip, not spacing. + // The one px length on a spacing property in the tree, and the reason + // .stylelintrc.json re-declares declaration-property-unit-allowed-list for + // this file with margin left out. A unit allowed-list cannot say "a pixel or + // two", which is what the negative margin is: the classic off-screen clip, + // not spacing. Padding, gap and font-size here are still held. margin: -1px; overflow: hidden; clip-path: inset(50%); diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index d5d9356..bce39fd 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -1801,11 +1801,14 @@ describe("the plugin rule sets the lint gate claims to run", () => { const { ESLint } = await import("eslint"); const react = (await import("eslint-plugin-react")).default; + // calculateConfigForFile answers for a path with nothing behind it, so a + // rename would otherwise leave this guard green over a file that moved. + const target = join(projectRoot, "src/components/DataTable/TableHead.tsx"); + expect(existsSync(target), "the guard's sample file moved").toBe(true); + const resolved: unknown = await new ESLint({ cwd: projectRoot, - }).calculateConfigForFile( - join(projectRoot, "src/components/DataTable/TableHead.tsx"), - ); + }).calculateConfigForFile(target); const active = (resolved as { rules?: Record }).rules ?? {}; From ad30191b1a3dc56ba9ee483e8aa9f3a3d25d9647 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:14:49 -0700 Subject: [PATCH 18/20] fix(search): strip the field separator from the needle The search key joins four fields with a NUL and the comment claimed no input could produce one. The address can: ?q=%00 survives trim and matches every row, and a crafted needle spanned two fields. Stripped where the needle is formed, so any caller is covered. --- src/api/getCities.test.ts | 14 ++++++++++++++ src/api/getCities.ts | 9 +++++++-- src/data/worldcities/cities.ts | 8 ++++++-- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/api/getCities.test.ts b/src/api/getCities.test.ts index d08b58c..68ceee3 100644 --- a/src/api/getCities.test.ts +++ b/src/api/getCities.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { SEARCH_KEY_SEPARATOR } from "../data/worldcities/cities"; import { getCities } from "./getCities"; import type { City } from "./getCities"; import { @@ -172,6 +173,19 @@ describe("getCities", () => { expect(await getCities({ searchTerm: "admin" })).toHaveLength(0); }); + it("cannot be made to match across two indexed fields", async () => { + // The address is an input path the box is not: ?q=%00 decodes to a real + // separator and trim leaves it, so a needle could span the join. getCities + // strips it, which turns a bare one into the empty term. + const spanning = await getCities({ + searchTerm: `Tokyo${SEARCH_KEY_SEPARATOR}Tokyo`, + }); + expect(spanning).toEqual(await getCities({ searchTerm: "TokyoTokyo" })); + + const bare = await getCities({ searchTerm: SEARCH_KEY_SEPARATOR }); + expect(bare).toHaveLength((await getCities({ searchTerm: "" })).length); + }); + it("returns an empty list when nothing matches", async () => { const result = await getCities({ searchTerm: "zzzzzzzz" }); expect(result).toHaveLength(0); diff --git a/src/api/getCities.ts b/src/api/getCities.ts index 88ee483..3103731 100644 --- a/src/api/getCities.ts +++ b/src/api/getCities.ts @@ -1,5 +1,5 @@ import type { City } from "../data/worldcities/cities"; -import { loadCities } from "../data/worldcities/cities"; +import { SEARCH_KEY_SEPARATOR, loadCities } from "../data/worldcities/cities"; // Re-exported so no consumer's import path changed when the definition moved. export type { City }; @@ -22,7 +22,12 @@ export async function getCities({ }: GetCitiesParams = {}): Promise { const all = await loadCities(); - const needle = searchTerm.trim().toLowerCase(); + // Stripped here rather than at the URL parser, so a needle arriving any + // other way cannot span two fields either. + const needle = searchTerm + .replaceAll(SEARCH_KEY_SEPARATOR, "") + .trim() + .toLowerCase(); // The empty term returns a copy, so the module-scope cache cannot escape. const matched = needle ? all.filter((city) => city.searchKey.includes(needle)) diff --git a/src/data/worldcities/cities.ts b/src/data/worldcities/cities.ts index 6dae5c8..f3afa7d 100644 --- a/src/data/worldcities/cities.ts +++ b/src/data/worldcities/cities.ts @@ -79,8 +79,12 @@ const COLUMNS = [ "population", ] as const; -/** A text input cannot produce this, so a needle cannot cross a field. */ -const SEARCH_KEY_SEPARATOR = "\u0000"; +/** + * Joins the indexed fields. The address can carry one (`?q=%00`) and trim does + * not strip it, so getCities removes it from the needle rather than this being + * a character no input produces. + */ +export const SEARCH_KEY_SEPARATOR = "\u0000"; /** A search cache rather than a fact about a city, so it stays off City. */ interface IndexedCity extends City { From 2fab71d38b23758a73a713309b1df1e8e94aae05 Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:46:39 -0700 Subject: [PATCH 19/20] ci: accept Chromatic baselines on main A build on the baseline branch does not accept its own snapshots, so the five the visual spec captures stayed unreviewed and every pull request since has re-reported them as changes against no baseline. --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e59f80..59dfa2f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -195,6 +195,10 @@ jobs: with: playwright: true exitOnceUploaded: true + # A build on the baseline branch does not accept its own snapshots. + # Without this they stay unreviewed, and a later pull request finds no + # accepted ancestor to compare against, so it re-reports every one. + autoAcceptChanges: main # A job rather than its own workflow, because `needs` is what makes the publish # wait on the gate. The write permissions sit here so the job running untrusted From 4f590b4d14149f069dbdbc4ec54a457193f701aa Mon Sep 17 00:00:00 2001 From: Norman Yee <155019+funkadelic@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:08:26 -0700 Subject: [PATCH 20/20] fix(search): answer nothing for a term carrying the field separator Deleting the separator answered a different search: a term of nothing but one returned every row, and an embedded one silently became the term without it. No field's content holds a separator, so such a term matches nothing. --- src/api/getCities.test.ts | 22 +++++++++++++++++----- src/api/getCities.ts | 25 ++++++++++++++++--------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/api/getCities.test.ts b/src/api/getCities.test.ts index 68ceee3..3c8503e 100644 --- a/src/api/getCities.test.ts +++ b/src/api/getCities.test.ts @@ -173,17 +173,29 @@ describe("getCities", () => { expect(await getCities({ searchTerm: "admin" })).toHaveLength(0); }); - it("cannot be made to match across two indexed fields", async () => { + it("matches nothing for a term carrying the field separator", async () => { // The address is an input path the box is not: ?q=%00 decodes to a real - // separator and trim leaves it, so a needle could span the join. getCities - // strips it, which turns a bare one into the empty term. + // separator and trim leaves it, so a term could otherwise span the join. + // No field's content holds a separator, so such a term matches nothing. const spanning = await getCities({ searchTerm: `Tokyo${SEARCH_KEY_SEPARATOR}Tokyo`, }); - expect(spanning).toEqual(await getCities({ searchTerm: "TokyoTokyo" })); + expect(spanning).toHaveLength(0); + // Not rewritten into a search for the term without it. + expect(await getCities({ searchTerm: "TokyoTokyo" })).toHaveLength(0); + const embedded = await getCities({ + searchTerm: `To${SEARCH_KEY_SEPARATOR}kyo`, + }); + expect(embedded).toHaveLength(0); + expect((await getCities({ searchTerm: "Tokyo" })).length).toBeGreaterThan( + 0, + ); + + // A term of nothing but a separator answers nothing, not every row. const bare = await getCities({ searchTerm: SEARCH_KEY_SEPARATOR }); - expect(bare).toHaveLength((await getCities({ searchTerm: "" })).length); + expect(bare).toHaveLength(0); + expect((await getCities({ searchTerm: "" })).length).toBeGreaterThan(0); }); it("returns an empty list when nothing matches", async () => { diff --git a/src/api/getCities.ts b/src/api/getCities.ts index 3103731..41f7d94 100644 --- a/src/api/getCities.ts +++ b/src/api/getCities.ts @@ -22,16 +22,23 @@ export async function getCities({ }: GetCitiesParams = {}): Promise { const all = await loadCities(); - // Stripped here rather than at the URL parser, so a needle arriving any - // other way cannot span two fields either. - const needle = searchTerm - .replaceAll(SEARCH_KEY_SEPARATOR, "") - .trim() - .toLowerCase(); + const needle = searchTerm.trim().toLowerCase(); + + // Answered here rather than at the URL parser, so a term arriving any other + // way is answered the same. The separator marks a field boundary in the + // search key, so no field's content holds one and a term carrying one matches + // nothing. Deleting it instead would answer a different search, and a term of + // nothing but a separator would answer every row. + // // The empty term returns a copy, so the module-scope cache cannot escape. - const matched = needle - ? all.filter((city) => city.searchKey.includes(needle)) - : [...all]; + let matched: City[]; + if (needle === "") { + matched = [...all]; + } else if (needle.includes(SEARCH_KEY_SEPARATOR)) { + matched = []; + } else { + matched = all.filter((city) => city.searchKey.includes(needle)); + } // Applied to the filter as well as the download, so a cache-warm call still // behaves like a network call and the debounce timing keeps its meaning.