From 798553b13b4e173a7066997ce598ebd6c3e9da2f Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 11:43:21 +0200 Subject: [PATCH] ci(client): lint:layers holds the import direction between src/ trees Components reach up into pages/ for models and helpers (28 imports in 25 files: the journey detail helpers, the collections, atlas and dashboard models) and sideways into mobile/ for phone hooks and sheets (36 in 30 files), and a few stores and a repo import from components/. Nothing stopped the next one, and every such import ties two trees together that should move and split independently. lint:layers reads each source file's imports from its syntax tree (multi-line imports, re-exports, import() and type imports included, nothing inside comments or strings) and checks them against a small rule table: a component imports neither pages/ nor mobile/, a shared hook no view, nothing under the views (store, repo, sync, api, db, utils and the other helpers) any view, a repo no store, and the Dexie schema none of the layers above it. mobile/ and pages/ stay free, since the phone screens reuse the page hooks by design. The 80 imports that break a rule today are counted per file in scripts/layers-baseline.json, and a file may hold no more than its entry. Type imports count: a type the lower layer needs belongs below it. Tests are not checked. --- client/CLAUDE.md | 2 + client/package.json | 1 + client/scripts/layer-lint.mjs | 25 ++++ client/scripts/layers-baseline.json | 65 +++++++++++ client/scripts/lib/imports.mjs | 70 +++++++++++ client/scripts/lib/layers.mjs | 96 +++++++++++++++ client/tests/unit/scripts/layer-lint.test.ts | 117 +++++++++++++++++++ 7 files changed, 376 insertions(+) create mode 100644 client/scripts/layer-lint.mjs create mode 100644 client/scripts/layers-baseline.json create mode 100644 client/scripts/lib/imports.mjs create mode 100644 client/scripts/lib/layers.mjs create mode 100644 client/tests/unit/scripts/layer-lint.test.ts diff --git a/client/CLAUDE.md b/client/CLAUDE.md index e6bd48ecc..04159bb08 100644 --- a/client/CLAUDE.md +++ b/client/CLAUDE.md @@ -13,6 +13,7 @@ npm run lint:pages # enforce the Page pattern (CI gate) npm run lint:rtl # physical left/right sides and rtl-lint-disable comments may only shrink per file (CI gate; --list shows them, --update lowers both baselines) npm run lint:size # no source file or stylesheet past 1000 lines, no test past 2000, counted wrapped at 120 columns; longer ones may only shrink (CI gate; --update lowers the baseline) npm run lint:format # Prettier: every file outside scripts/format-baseline.json must be formatted, the list only shrinks (CI gate; npx prettier --write , then --update) +npm run lint:layers # imports go downwards: components never import pages/ or mobile/, nothing under the views imports a view; per-file counts may only shrink (CI gate; --list, --update) npm run test # vitest run (tests/** + co-located src/**/*.test.{ts,tsx}); also test:unit / test:integration / test:coverage npm run e2e # Playwright (CI runs --project=public --project=app; e2e:report opens the last report) npm run shots # Playwright screenshot project (shots:promote to accept) @@ -44,6 +45,7 @@ The offline core is flagship work surrounded by a periphery that ignores it. New - **One data path, no layer skips.** Never import `api/client` (or raw fetch/axios) from a component or modal — especially never *write* from one. A long tail of existing files bypasses the layering and no lint ratchet holds the line yet; that is debt, not precedent. - **Offline-first is the product promise.** New domains get a repo with read-through and offline writes (temp id → optimistic Dexie write → idempotent queue entry). If a domain is deliberately online-only, say so explicitly. +- **Imports go downwards** (`lint:layers`, rules in `scripts/lib/layers.mjs`). A component never imports `pages/` or `mobile/`, a shared hook no view, and stores, repos, sync, api, db and the helpers no view at all. A model or helper both sides need moves down (`utils/`, `hooks/`, `types/`); the counts in `scripts/layers-baseline.json` are debt, not precedent. - **No god components or god hooks.** The page pattern is a floor, not a ceiling. Decompose hooks by concern, split renders into memoizable sections, and don't pass huge prop bags with inline-arrow callbacks. - **Prefer React 19 idioms**: `useOptimistic`, `useActionState`/`useFormStatus`, `use()` + Suspense, `useSyncExternalStore` for external subscriptions (`hooks/useNetworkMode` is the reference). - **Optimistic writes must reconcile** — rollback + user-visible toast on failure. No `.catch(() => {})`, no bare `catch {}`. diff --git a/client/package.json b/client/package.json index 71174491c..e96c829f0 100644 --- a/client/package.json +++ b/client/package.json @@ -21,6 +21,7 @@ "lint:rtl": "node scripts/rtl-lint.mjs", "lint:size": "node scripts/size-lint.mjs", "lint:format": "node scripts/format-lint.mjs", + "lint:layers": "node scripts/layer-lint.mjs", "check:gl-split": "node scripts/check-gl-split.mjs", "theme:lint": "node scripts/theme-lint.mjs", "theme:lint:strict": "node scripts/theme-lint.mjs --strict", diff --git a/client/scripts/layer-lint.mjs b/client/scripts/layer-lint.mjs new file mode 100644 index 000000000..1b5028f23 --- /dev/null +++ b/client/scripts/layer-lint.mjs @@ -0,0 +1,25 @@ +#!/usr/bin/env node +/* + * lint:layers: imports follow the layering, downwards only. + * + * Components reached up into pages/ for models and helpers, and sideways into + * mobile/ for phone hooks and sheets, so nothing could be moved or split + * without touching three trees. scripts/lib/layers.mjs holds the rules: a + * component imports neither pages/ nor mobile/, a shared hook no view, and + * nothing under the views (stores, repos, sync, api, db, helpers) a view at + * all. The imports that already broke a rule are counted per file in + * scripts/layers-baseline.json, and a file may hold no more of them than its + * entry (a file without an entry holds none). Type-only imports count: a type + * the lower layer needs belongs below it. + * + * npm run lint:layers check against the baseline (CI) + * npm run lint:layers -- --list print every import against the layering + * npm run lint:layers -- --update lower the baseline to what the files hold now; + * it never raises an entry + */ +import { fileURLToPath } from 'node:url' +import { check } from './lib/layers.mjs' +import { runCli } from './lib/ratchet.mjs' + +const root = fileURLToPath(new URL('..', import.meta.url)) +await runCli('lint:layers', (args) => check({ root, update: args.includes('--update'), list: args.includes('--list') })) diff --git a/client/scripts/layers-baseline.json b/client/scripts/layers-baseline.json new file mode 100644 index 000000000..565fe4aee --- /dev/null +++ b/client/scripts/layers-baseline.json @@ -0,0 +1,65 @@ +{ + "src/api/client.ts": 1, + "src/components/Atlas/AtlasCountryPlaces.tsx": 1, + "src/components/Budget/ReceiptScanModal.tsx": 1, + "src/components/Collab/CollabLinks.tsx": 1, + "src/components/Collab/CollabNotes.tsx": 1, + "src/components/Collections/AddPlaceToCollectionModal.tsx": 1, + "src/components/Collections/CollectionFilterBar.tsx": 2, + "src/components/Collections/CollectionLabelFilter.tsx": 1, + "src/components/Collections/CollectionMap.tsx": 1, + "src/components/Collections/CollectionPicker.tsx": 1, + "src/components/Collections/CollectionPlaceDetail.tsx": 1, + "src/components/Collections/ListEditorModal.tsx": 1, + "src/components/Collections/MSaveToCollectionSheet.tsx": 3, + "src/components/Collections/StatusBadge.tsx": 1, + "src/components/Dawarich/DawarichAcceptDialog.tsx": 3, + "src/components/Dawarich/DawarichAtlasDialog.tsx": 3, + "src/components/Dawarich/DawarichSuggestionsPanel.tsx": 1, + "src/components/Journey/ContributorInviteDialog.tsx": 1, + "src/components/Journey/JourneyDetailPageAddTripDialog.tsx": 1, + "src/components/Journey/JourneyDetailPageChips.tsx": 1, + "src/components/Journey/JourneyDetailPageEntryCard.tsx": 2, + "src/components/Journey/JourneyDetailPageEntryEditor.tsx": 3, + "src/components/Journey/JourneyDetailPageGalleryView.tsx": 1, + "src/components/Journey/JourneyDetailPageMapView.tsx": 1, + "src/components/Journey/JourneyDetailPagePhotoGrid.tsx": 1, + "src/components/Journey/JourneyDetailPageProviderPicker.tsx": 2, + "src/components/Journey/JourneyDetailPageSettingsDialog.tsx": 1, + "src/components/Journey/JourneyEntryCover.tsx": 2, + "src/components/Journey/MobileEntryView.tsx": 1, + "src/components/Map/MapView.tsx": 1, + "src/components/Map/MapViewGL.tsx": 1, + "src/components/Packing/packingPrintDocument.ts": 1, + "src/components/Packing/usePackingExport.ts": 1, + "src/components/Planner/IcsSubscribeModal.tsx": 1, + "src/components/Planner/TransitJourneyModal.tsx": 1, + "src/components/Planner/TransitSearchPanel.tsx": 1, + "src/components/Planner/TripWarningsBanner.tsx": 1, + "src/components/Planner/bookings/bookingsModel.ts": 2, + "src/components/Planner/transitDisplay.tsx": 1, + "src/components/Plugins/PluginFrame.tsx": 1, + "src/components/Roadtrip/RoadtripSidebarDriveBands.tsx": 1, + "src/components/Roadtrip/RoadtripSidebarRefuelBand.tsx": 1, + "src/components/Studio/StudioShell.tsx": 1, + "src/components/Studio/StudioSidebar.tsx": 1, + "src/components/Studio/TravelElements.tsx": 1, + "src/components/Todo/useTodoList.ts": 1, + "src/components/Trips/TripMembersModal.tsx": 1, + "src/components/shared/ConfirmDialog.tsx": 1, + "src/components/shared/CountryFlag.tsx": 1, + "src/components/shared/EmptyState.tsx": 1, + "src/components/shared/TripLoadingSplash.tsx": 1, + "src/components/shared/dialogFocus.ts": 1, + "src/hooks/useDawarichConnection.ts": 1, + "src/hooks/useDawarichJournalTrail.ts": 1, + "src/hooks/useDawarichSuggestions.ts": 1, + "src/hooks/useDayNotes.ts": 1, + "src/hooks/useRouteCalculation.ts": 4, + "src/hooks/useTransportRoutes.ts": 1, + "src/hooks/useWebPush.ts": 1, + "src/repo/pluginPoiRepo.ts": 1, + "src/store/slices/budgetSlice.ts": 1, + "src/store/studioStore.ts": 2, + "src/store/vacayStore.ts": 1 +} diff --git a/client/scripts/lib/imports.mjs b/client/scripts/lib/imports.mjs new file mode 100644 index 000000000..f3821176e --- /dev/null +++ b/client/scripts/lib/imports.mjs @@ -0,0 +1,70 @@ +/* + * The imports of a source file, read from its syntax tree rather than with a + * regular expression, so an import split over lines, a dynamic import() and a + * re-export count, and an import inside a comment or a string does not. + */ +import { dirname, join, relative, resolve } from 'node:path' +import ts from 'typescript' +import { listFiles, readText, TEST_FILE, toKey } from './ratchet.mjs' + +const bindingsAreTypes = (elements) => elements.length > 0 && elements.every((el) => el.isTypeOnly) + +/** + * Every module the file names, with the line it is named on and whether the + * import only brings in types (erased from the bundle, so no runtime path). + */ +export function importsOf(source, file) { + const kind = file.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS + const sf = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, kind) + const found = [] + const add = (node, literal, typeOnly) => { + if (!literal || !ts.isStringLiteralLike(literal)) return + found.push({ specifier: literal.text, line: sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1, typeOnly }) + } + const visit = (node) => { + if (ts.isImportDeclaration(node)) { + const clause = node.importClause + const named = clause?.namedBindings && ts.isNamedImports(clause.namedBindings) ? clause.namedBindings.elements : null + const typeOnly = Boolean(clause && (clause.isTypeOnly || (!clause.name && named && bindingsAreTypes(named)))) + add(node, node.moduleSpecifier, typeOnly) + } else if (ts.isExportDeclaration(node) && node.moduleSpecifier) { + const named = node.exportClause && ts.isNamedExports(node.exportClause) ? node.exportClause.elements : null + add(node, node.moduleSpecifier, node.isTypeOnly || Boolean(named && bindingsAreTypes(named))) + } else if (ts.isImportEqualsDeclaration(node) && ts.isExternalModuleReference(node.moduleReference)) { + add(node, node.moduleReference.expression, node.isTypeOnly) + } else if (ts.isImportTypeNode(node) && ts.isLiteralTypeNode(node.argument)) { + add(node, node.argument.literal, true) + } else if ( + ts.isCallExpression(node) && + (node.expression.kind === ts.SyntaxKind.ImportKeyword || + (ts.isIdentifier(node.expression) && node.expression.text === 'require')) + ) { + add(node, node.arguments[0], false) + } + ts.forEachChild(node, visit) + } + visit(sf) + return found +} + +/** + * The top directory under src/ a relative import lands in ("components", + * "api"; a file directly in src/ gives its own name), or null for a package + * or a path outside src/. + */ +export function layerOf(srcRoot, fromFile, specifier) { + if (!specifier.startsWith('.')) return null + const target = relative(srcRoot, resolve(dirname(fromFile), specifier)).split('\\').join('/') + if (!target || target.startsWith('..')) return null + return target.split('/')[0] +} + +/** Every source file under root/src outside the tests, with its key, its layer and its imports resolved to layers. */ +export function sourceGraph(root) { + const src = join(root, 'src') + return listFiles(root, ['src'], (key) => /\.tsx?$/.test(key) && !TEST_FILE.test(key)).map((path) => ({ + key: toKey(root, path), + layer: toKey(src, path).split('/')[0], + imports: importsOf(readText(path), path).map((imp) => ({ ...imp, layer: layerOf(src, path, imp.specifier) })), + })) +} diff --git a/client/scripts/lib/layers.mjs b/client/scripts/lib/layers.mjs new file mode 100644 index 000000000..6cdcd4154 --- /dev/null +++ b/client/scripts/lib/layers.mjs @@ -0,0 +1,96 @@ +/* + * The rules and the comparison behind lint:layers (scripts/layer-lint.mjs). + */ +import { join } from 'node:path' +import { countMap, lowerCounts, readBaseline, writeBaseline } from './ratchet.mjs' +import { sourceGraph } from './imports.mjs' + +const VIEW = ['components', 'mobile', 'pages', 'hooks'] + +/** + * For each directory under src/, the directories its files may not import + * from. The view layers sit on top: a page composes components, a component + * may use a shared hook, and the phone screens reuse both. Nothing below the + * views (stores, repos, sync, api, the Dexie schema and the helpers) knows a + * view exists, a repo sits under the stores, and the Dexie schema under all + * of them. Directories without an entry (mobile, pages, the app root) are + * free. + */ +export const RULES = { + components: ['pages', 'mobile'], + hooks: ['pages', 'mobile', 'components'], + store: VIEW, + sync: VIEW, + api: VIEW, + utils: VIEW, + i18n: VIEW, + theme: VIEW, + constants: VIEW, + services: VIEW, + push: VIEW, + types: VIEW, + repo: [...VIEW, 'store'], + db: [...VIEW, 'store', 'repo', 'sync', 'api'], +} + +/** + * Every import that crosses a boundary the wrong way, per file. A type-only + * import counts too: a type the lower layer needs belongs below it. + */ +export function violations(root, rules = RULES) { + const found = {} + for (const file of sourceGraph(root)) { + const banned = rules[file.layer] + if (!banned) continue + const wrong = file.imports.filter((imp) => imp.layer && banned.includes(imp.layer)) + if (wrong.length) found[file.key] = wrong.map((imp) => `${imp.line}: '${imp.specifier}' (${file.layer} into ${imp.layer})`) + } + return found +} + +/** Runs the check against the baseline at baselinePath and returns the exit code. */ +export function check({ + root, + baselinePath = join(root, 'scripts/layers-baseline.json'), + rules = RULES, + update = false, + list = false, + log = console.log, + error = console.error, +}) { + const found = violations(root, rules) + const counts = Object.fromEntries(Object.entries(found).map(([key, list]) => [key, list.length])) + let baseline = readBaseline(baselinePath, countMap) + + if (list) { + for (const [key, wrong] of Object.entries(found)) { + log(`${key} (${wrong.length}, baseline ${baseline[key] ?? 0})`) + for (const line of wrong) log(` ${line}`) + } + } + + if (update) { + baseline = lowerCounts(baseline, counts) + writeBaseline(baselinePath, baseline) + } + + const grown = Object.entries(counts).filter(([key, n]) => n > (baseline[key] ?? 0)) + const lowerable = Object.entries(baseline).filter(([key, n]) => (counts[key] ?? 0) < n) + + for (const [key, n] of grown) { + error(`FAIL ${key}: ${n} import(s) against the layering, baseline ${baseline[key] ?? 0}:`) + for (const line of found[key]) error(` ${line}`) + } + if (grown.length) { + error( + 'Move what both sides need down into a layer they may share (a model or helper under utils/, ' + + 'a hook under hooks/, a type under types/) instead of importing upwards.', + ) + } + if (lowerable.length && !update) { + log(`${lowerable.length} file(s) hold fewer of these imports than the baseline: run with --update to lower it.`) + } + const sum = (map) => Object.values(map).reduce((a, b) => a + b, 0) + log(`layers: ${sum(counts)} import(s) against the layering in ${Object.keys(counts).length} file(s), baseline allows ${sum(baseline)}`) + return grown.length ? 1 : 0 +} diff --git a/client/tests/unit/scripts/layer-lint.test.ts b/client/tests/unit/scripts/layer-lint.test.ts new file mode 100644 index 000000000..5053513b1 --- /dev/null +++ b/client/tests/unit/scripts/layer-lint.test.ts @@ -0,0 +1,117 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { importsOf, layerOf } from '../../../scripts/lib/imports.mjs'; +import { check, RULES, violations } from '../../../scripts/lib/layers.mjs'; +import { RatchetError } from '../../../scripts/lib/ratchet.mjs'; +import { ratchetTree, type RatchetTree } from '../../helpers/ratchetFixture'; + +let tree: RatchetTree; +afterEach(() => tree?.remove()); + +const BASELINE = 'scripts/layers-baseline.json'; + +function run(baseline: Record, files: Record, update = false) { + tree = ratchetTree({ [BASELINE]: JSON.stringify(baseline), ...files }); + return check({ root: tree.root, baselinePath: tree.path(BASELINE), update, ...tree.out }); +} + +describe('scripts/lib/imports', () => { + it('IMPORTS-001: finds static, multi-line, re-exported, dynamic and type imports, and nothing in comments or strings', () => { + const source = [ + "import a from './a'", + 'import {', + ' b,', + "} from './b'", + "import type { C } from './c'", + "import { type D } from './d'", + "import E, { type F } from './e'", + "export { g } from './g'", + "export type { H } from './h'", + "const i = () => import('./i')", + "let j: typeof import('./j')", + "// import k from './k'", + 'const l = "import m from \'./m\'"', + "import './n.css'", + ].join('\n'); + expect(importsOf(source, 'x.tsx')).toEqual([ + { specifier: './a', line: 1, typeOnly: false }, + { specifier: './b', line: 2, typeOnly: false }, + { specifier: './c', line: 5, typeOnly: true }, + { specifier: './d', line: 6, typeOnly: true }, + { specifier: './e', line: 7, typeOnly: false }, + { specifier: './g', line: 8, typeOnly: false }, + { specifier: './h', line: 9, typeOnly: true }, + { specifier: './i', line: 10, typeOnly: false }, + { specifier: './j', line: 11, typeOnly: true }, + { specifier: './n.css', line: 14, typeOnly: false }, + ]); + }); + + it('IMPORTS-002: resolves a relative import to the top directory under src/', () => { + const src = join('/r', 'src'); + const from = join(src, 'components', 'Map', 'MapView.tsx'); + expect(layerOf(src, from, '../../pages/atlas/atlasModel')).toBe('pages'); + expect(layerOf(src, from, './MapMarkers')).toBe('components'); + expect(layerOf(src, from, '../../types')).toBe('types'); + expect(layerOf(src, from, 'react')).toBeNull(); + expect(layerOf(src, from, '../../../../shared/src')).toBeNull(); + }); +}); + +describe('lint:layers', () => { + it('LAYERS-001: components may not import pages/ or mobile/, and the core no view at all', () => { + expect(RULES.components).toEqual(expect.arrayContaining(['pages', 'mobile'])); + for (const core of ['store', 'repo', 'sync', 'api', 'db', 'utils']) { + expect(RULES[core as keyof typeof RULES]).toEqual( + expect.arrayContaining(['components', 'mobile', 'pages', 'hooks']) + ); + } + expect(RULES.repo).toContain('store'); + }); + + it('LAYERS-002: reports each import against the layering with its line', () => { + tree = ratchetTree({ + 'src/components/A.tsx': + "import { m } from '../pages/x/model'\nimport { useIsPhone } from '../mobile/useIsPhone'\nimport { s } from '../store/s'\n", + 'src/store/s.ts': "import type { P } from '../components/P'\n", + 'src/pages/P.tsx': "import A from '../components/A'\nimport M from '../mobile/M'\n", + 'src/components/A.test.tsx': "import P from '../pages/P'\n", + }); + expect(violations(tree.root)).toEqual({ + 'src/components/A.tsx': [ + "1: '../pages/x/model' (components into pages)", + "2: '../mobile/useIsPhone' (components into mobile)", + ], + 'src/store/s.ts': ["1: '../components/P' (store into components)"], + }); + }); + + it('LAYERS-003: fails a file that gains an import against the layering', () => { + expect(run({}, { 'src/components/A.tsx': "import { m } from '../pages/x/model'\n" })).toBe(1); + expect(tree.error.join('\n')).toMatch(/src\/components\/A\.tsx: 1 import\(s\) against the layering, baseline 0/); + expect(tree.error.join('\n')).toMatch(/1: '\.\.\/pages\/x\/model' \(components into pages\)/); + }); + + it('LAYERS-004: holds a baselined file at its entry', () => { + const files = { 'src/components/A.tsx': "import { m } from '../pages/x/model'\n" }; + expect(run({ 'src/components/A.tsx': 1 }, files)).toBe(0); + }); + + it('LAYERS-005: --update lowers and drops entries and never raises one', () => { + run( + { 'src/components/A.tsx': 3, 'src/components/B.tsx': 1 }, + { + 'src/components/A.tsx': "import { m } from '../pages/x/model'\n", + 'src/components/B.tsx': "import { m } from '../utils/m'\n", + }, + true + ); + expect(JSON.parse(readFileSync(tree.path(BASELINE), 'utf8'))).toEqual({ 'src/components/A.tsx': 1 }); + }); + + it('LAYERS-006: a missing baseline stops the check', () => { + tree = ratchetTree({ 'src/a.ts': '' }); + expect(() => check({ root: tree.root, baselinePath: tree.path(BASELINE), ...tree.out })).toThrow(RatchetError); + }); +});