-
-
Notifications
You must be signed in to change notification settings - Fork 97
fix(assets): finish #4111 — dedupe/optimize the logo and add an asset-usage gate #4139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9467c84
7ebe2d8
4ee23e9
a0a8e7c
996962c
e62ccdc
2fbc6cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@ifc-lite/viewer": patch | ||
| --- | ||
|
|
||
| Delete four more unreferenced favicon originals under `apps/viewer/public` left over from the same cleanup as #4111/#4114, and losslessly recompress `logo.png` (pixel-identical, verified) from 1.39 MB to 1.26 MB. `apps/landing/assets/logo.png` and `docs/assets/logo.png` — byte-identical copies of the same logo, each required by a genuinely separate deployment (the standalone landing site and the mkdocs docs build) — are recompressed the same way for consistency, though neither ships as part of the viewer bundle. | ||
|
|
||
| Also widen the new asset-usage gate's `TEXT_EXTENSIONS` to include `.mts`/`.cts`: `tools/demo-kit/derive-variants.mts` builds `apps/viewer/public/samples/*` paths, and until now the gate's text scan skipped that file, so removing the one other (redundant) mention of a sample name would have made the gate call a live asset dead. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| #!/usr/bin/env node | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Guard: nothing may land in apps/viewer/public that no code, markup or | ||
| * manifest anywhere in the repo actually points at. | ||
| * | ||
| * BACKGROUND (#4111). Five favicon variants sat in apps/viewer/public for | ||
| * months, 1.885 MiB shipped to every visitor for nothing — dead weight that | ||
| * a lone `grep` in a maintainer's issue found and #4114 deleted. Nothing | ||
| * caught that they were dead when they were added, and nothing would catch | ||
| * the next one. This is that check: it fails when apps/viewer/public gains a | ||
| * file that no other tracked text file in the repo references. | ||
| * | ||
| * WHAT COUNTS AS A REFERENCE. A substring search, not a parsed reference | ||
| * graph — see scripts/lib/asset-usage.mjs's findUnreferencedAssets for the | ||
| * exact candidates it tries (basename, the scan-relative path, and that path | ||
| * with a leading "/"). It scans EVERY tracked text file, not just app | ||
| * source: apps/viewer/index.html, apps/viewer/public/manifest.json, root | ||
| * vercel.json, docs, and E2E specs all count as consumers. Deliberately | ||
| * permissive — a missed dead file is a much smaller problem than a live one | ||
| * flagged as dead and deleted by a future PR that trusts this gate. | ||
| * | ||
| * WHAT IT CANNOT SEE. It is lexical: a path built at runtime by string | ||
| * concatenation or a template literal (`` `/favicon-${size}.png` ``) only | ||
| * matches if the literal pieces happen to contain a whole candidate string. | ||
| * None of the current viewer code does this (checked by hand when this gate | ||
| * was added), but a future refactor that introduces one would need an | ||
| * ALLOWLIST entry, same as any other false positive. | ||
| * | ||
| * ALLOWLIST. Some files are fetched by convention — a browser or crawler | ||
| * requests them by a fixed name with no in-repo link ever pointing at them | ||
| * (favicon.ico, apple-touch-icon.png, robots.txt). Those get an explicit, | ||
| * reasoned row below instead of silently passing or permanently failing. | ||
| * Adding a row is a real claim a reviewer can see and question, exactly | ||
| * like the module-size gate's ALLOWLIST — it is not a way to make the gate | ||
| * stop looking, it is a way to say "this one has a reason". | ||
| */ | ||
|
|
||
| import { execFileSync } from 'node:child_process'; | ||
| import { readFileSync } from 'node:fs'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { dirname, join, relative } from 'node:path'; | ||
| import { findUnreferencedAssets, TEXT_EXTENSIONS } from './lib/asset-usage.mjs'; | ||
|
|
||
| const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); | ||
|
|
||
| // Scoped to apps/viewer/public per #4111 — the SPA that actually shipped the | ||
| // dead favicons. Not generalized to every app's static directory: apps/landing | ||
| // and docs/assets are plain static trees with no build step to silently | ||
| // accumulate unreviewed output into, and widening scope without a concrete | ||
| // second incident to point at is exactly the kind of speculative generality | ||
| // AGENTS.md asks gates to avoid. | ||
| const SCAN_DIR = 'apps/viewer/public'; | ||
|
|
||
| // TEXT_EXTENSIONS lives in ./lib/asset-usage.mjs so check-asset-usage.test.mjs | ||
| // can assert on it directly instead of round-tripping through a real git | ||
| // checkout. | ||
|
|
||
| // Convention-fetched paths (relative to SCAN_DIR) that legitimately have no | ||
| // in-repo reference. Each row needs a reason a reviewer can check. | ||
| const ALLOWLIST = [ | ||
| // Browsers request /favicon.ico directly, with no <link> tag required. | ||
| // This repo's index.html happens to link it too, but that is not | ||
| // guaranteed to stay true, and the request happens either way. | ||
| 'favicon.ico', | ||
| // iOS Safari (and other UAs) request this exact path when a page is | ||
| // added to the home screen, independent of any <link rel="apple-touch-icon">. | ||
| 'apple-touch-icon.png', | ||
| // Crawlers request /robots.txt by convention; nothing in-repo ever needs | ||
| // to spell its name. Not present today, allowlisted for when it lands. | ||
| 'robots.txt', | ||
| // Same convention as robots.txt: crawlers and IDEs fetch it by fixed name. | ||
| 'sitemap.xml', | ||
| ]; | ||
|
|
||
| function git(args) { | ||
| return execFileSync('git', args, { cwd: ROOT, encoding: 'utf-8', maxBuffer: 64 * 1024 * 1024 }); | ||
| } | ||
|
|
||
| function listTrackedFiles(pathspec) { | ||
| const args = ['ls-files', '-z']; | ||
| if (pathspec) args.push('--', pathspec); | ||
| const raw = git(args); | ||
| return raw.split('\0').filter(Boolean); | ||
| } | ||
|
|
||
| const assetFiles = listTrackedFiles(SCAN_DIR); | ||
| if (assetFiles.length === 0) { | ||
| console.error(`❌ ${SCAN_DIR} has no tracked files (or does not exist). This check's scan dir | ||
| is stale — fix SCAN_DIR in scripts/check-asset-usage.mjs, don't ignore this.`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const assetPaths = assetFiles.map((p) => relative(SCAN_DIR, p)); | ||
|
|
||
| const allFiles = listTrackedFiles(); | ||
| const corpusFiles = []; | ||
| for (const p of allFiles) { | ||
| const dot = p.lastIndexOf('.'); | ||
| const ext = dot === -1 ? '' : p.slice(dot); | ||
| if (!TEXT_EXTENSIONS.has(ext)) continue; | ||
| let content; | ||
| try { | ||
| content = readFileSync(join(ROOT, p), 'utf-8'); | ||
| } catch { | ||
| continue; // deleted-but-still-in-index, a symlink, or non-utf8 — skip, don't crash the gate | ||
| } | ||
| corpusFiles.push({ path: p, content }); | ||
| } | ||
|
|
||
| const { unreferenced, allowlisted } = findUnreferencedAssets({ assetPaths, corpusFiles, allowlist: ALLOWLIST }); | ||
|
|
||
| if (unreferenced.length === 0) { | ||
| console.log(`✅ Every tracked file under ${SCAN_DIR} (${assetPaths.length} files) is referenced somewhere in the repo` + | ||
| (allowlisted.length > 0 ? `, except ${allowlisted.length} allowlisted convention-fetched file(s): ${allowlisted.join(', ')}.` : '.')); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.error(`❌ ${unreferenced.length} file(s) under ${SCAN_DIR} have no reference anywhere in the repo: | ||
| ${unreferenced.map((p) => ` - ${p}`).join('\n')} | ||
|
|
||
| If genuinely dead, delete the file(s) (this is what #4111/#4114 did for five | ||
| leftover favicons). If it is fetched by convention (favicon.ico, robots.txt, | ||
| a manifest icon) with no in-repo link, add a reasoned row to ALLOWLIST in | ||
| scripts/check-asset-usage.mjs instead of ignoring this.`); | ||
|
|
||
| process.exit(1); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Tests for scripts/lib/asset-usage.mjs, the detection logic behind | ||
| * check-asset-usage.mjs (#4111). Runs entirely against synthetic asset/corpus | ||
| * lists, never against this checkout's own apps/viewer/public, so a future | ||
| * change to the repo's real assets can never make these vacuously pass — | ||
| * same reasoning as check-refwalk-guards.test.mjs's synthetic Rust trees. | ||
| * | ||
| * Run: `node --test scripts/check-asset-usage.test.mjs` | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use the root test command in both locations. The new documentation and CI step invoke
As per coding guidelines, “Always run typecheck/test through the root 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| */ | ||
|
|
||
| import test from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { findUnreferencedAssets, TEXT_EXTENSIONS } from './lib/asset-usage.mjs'; | ||
|
|
||
| test('a file mentioned by basename elsewhere in the repo is referenced', () => { | ||
| const { unreferenced } = findUnreferencedAssets({ | ||
| assetPaths: ['logo.png'], | ||
| corpusFiles: [{ path: 'apps/viewer/src/App.tsx', content: 'src="/logo.png"' }], | ||
| allowlist: [], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| }); | ||
|
|
||
| test('a file nothing mentions is reported unreferenced', () => { | ||
| const { unreferenced } = findUnreferencedAssets({ | ||
| assetPaths: ['favicon-16x16.png'], | ||
| corpusFiles: [{ path: 'apps/viewer/index.html', content: '<link href="/favicon-32x32.png">' }], | ||
| allowlist: [], | ||
| }); | ||
| assert.deepEqual(unreferenced, ['favicon-16x16.png']); | ||
| }); | ||
|
|
||
| test('an allowlisted convention-fetched file with no reference is not reported', () => { | ||
| const { unreferenced, allowlisted } = findUnreferencedAssets({ | ||
| assetPaths: ['favicon.ico', 'robots.txt'], | ||
| corpusFiles: [{ path: 'apps/viewer/index.html', content: 'nothing here mentions either' }], | ||
| allowlist: ['favicon.ico', 'robots.txt'], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| assert.deepEqual(allowlisted.sort(), ['favicon.ico', 'robots.txt']); | ||
| }); | ||
|
|
||
| test('an allowlist row for a file that IS referenced does not suppress anything spuriously', () => { | ||
| // Allowlisting a file that turns out to be referenced anyway is harmless — | ||
| // referenced files are never reported regardless of the allowlist. | ||
| const { unreferenced, allowlisted } = findUnreferencedAssets({ | ||
| assetPaths: ['manifest.json'], | ||
| corpusFiles: [{ path: 'apps/viewer/index.html', content: '<link rel="manifest" href="/manifest.json">' }], | ||
| allowlist: ['manifest.json'], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| assert.deepEqual(allowlisted, []); // referenced, so never even reaches the allowlist branch | ||
| }); | ||
|
|
||
| test('a nested path is matched by its root-absolute form', () => { | ||
| // `path` here is a label only -- findUnreferencedAssets matches on | ||
| // `content`, never on the corpus entry's `path` (see the implementation). | ||
| // Deliberately a synthetic, not-in-repo name (a real vercel.json exists at | ||
| // the repo root and would otherwise make check-ci-path-coverage.mjs treat | ||
| // this literal as a live input the gate reads, which it does not). | ||
| const { unreferenced } = findUnreferencedAssets({ | ||
| assetPaths: ['oauth/bcf/callback.html'], | ||
| corpusFiles: [{ path: 'fixture-rewrite-config.json', content: '"destination": "/oauth/bcf/callback.html"' }], | ||
| allowlist: [], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| }); | ||
|
|
||
| test('a nested path is matched by its bare scan-relative form', () => { | ||
| const { unreferenced } = findUnreferencedAssets({ | ||
| assetPaths: ['samples/hello-wall.ifc'], | ||
| corpusFiles: [{ path: 'apps/viewer/src/samples.ts', content: "loadSample('samples/hello-wall.ifc')" }], | ||
| allowlist: [], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| }); | ||
|
|
||
| test('empty asset list reports nothing (not a vacuous pass the CLI would hide)', () => { | ||
| const { unreferenced, allowlisted } = findUnreferencedAssets({ assetPaths: [], corpusFiles: [], allowlist: [] }); | ||
| assert.deepEqual(unreferenced, []); | ||
| assert.deepEqual(allowlisted, []); | ||
| }); | ||
|
|
||
| test('TEXT_EXTENSIONS covers .mts/.cts (a live asset can be referenced only from a build script)', () => { | ||
| // tools/demo-kit/derive-variants.mts constructs apps/viewer/public/samples/* | ||
| // paths at runtime; today those names are ALSO spelled out verbatim in | ||
| // apps/viewer/src/lib/tours/demo-kit.ts and AGENTS.md, both already-covered | ||
| // extensions, so the gate currently passes either way. But if that | ||
| // redundant mention were ever removed while the .mts generator remained | ||
| // the only reference, a scan that skips .mts would call a live asset dead | ||
| // and false-positive the gate. Assert the extensions directly, since | ||
| // findUnreferencedAssets itself is extension-agnostic (the CLI wrapper | ||
| // does the filtering) — this is the check that would have caught the gap. | ||
| assert.ok(TEXT_EXTENSIONS.has('.mts'), '.mts must be scanned (e.g. tools/demo-kit/derive-variants.mts)'); | ||
| assert.ok(TEXT_EXTENSIONS.has('.cts'), '.cts is the same TypeScript-module-flavor as .mts'); | ||
| }); | ||
|
|
||
| test('a substring match inside an unrelated word still counts (permissive by design)', () => { | ||
| // Documents the known false-negative direction: this is a substring search, | ||
| // not a parsed reference graph, and it is meant to err this way — see the | ||
| // "WHAT IT CANNOT SEE" note in check-asset-usage.mjs. | ||
| const { unreferenced } = findUnreferencedAssets({ | ||
| assetPaths: ['icon.png'], | ||
| corpusFiles: [{ path: 'notes.md', content: 'this word contains icon.png as a substring, not a real link' }], | ||
| allowlist: [], | ||
| }); | ||
| assert.deepEqual(unreferenced, []); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Pure detection logic for check-asset-usage.mjs, split out so it can be | ||
| * tested against synthetic fixtures instead of this checkout's own state | ||
| * (the module-size and refwalk gates split the same way, for the same | ||
| * reason: the CLI wrapper drives real `git ls-files`, the test drives this). | ||
| * | ||
| * An asset under the scanned directory (apps/viewer/public, at the call | ||
| * site) is "referenced" if some OTHER tracked text file in the repo — any | ||
| * file, not just source: index.html, manifest.json, vercel.json, docs, | ||
| * CSS, E2E specs all count — contains the asset's basename, its | ||
| * scan-relative path, or that path with a leading "/" (the root-absolute | ||
| * form a static-asset directory is served under). That is a substring | ||
| * search, not a parsed reference graph: it is deliberately permissive, so | ||
| * the gate's failure mode is a missed dead file, never a live one flagged | ||
| * as dead. | ||
| */ | ||
|
|
||
| /** | ||
| * Extensions worth reading as text for the substring search. Deliberately | ||
| * broad — docs (.md), configs (.json/.yml/.toml), markup (.html) and styles | ||
| * (.css) have all been real consumers in this repo's history. `.mts`/`.cts` | ||
| * are TypeScript too (tools/demo-kit/derive-variants.mts builds | ||
| * apps/viewer/public/samples/* paths at lines ~116-120) — omitting them left | ||
| * a live-asset-flagged-dead trap: the only thing stopping a false positive | ||
| * today is that those sample names are also spelled out in | ||
| * apps/viewer/src/lib/tours/demo-kit.ts and AGENTS.md, both already-covered | ||
| * extensions. `.sh`/`.py` are deliberately NOT added here: none of them | ||
| * reference a path under apps/viewer/public today (checked by grepping this | ||
| * repo's tracked .sh/.py files for every current asset's basename), so | ||
| * adding them would only cost scan time for an extension this scan dir has | ||
| * no current consumer in. `.rs` is NOT added either, but for a different | ||
| * reason: two Rust tests (rust/processing/tests/instancing_dont_bake.rs and | ||
| * rust/geometry/tests/clash_intersection_real_model.rs) do reference | ||
| * apps/viewer/public sample paths, so omitting `.rs` is a live gap, not an | ||
| * empty one — it stays safe only because both referenced assets | ||
| * (hello-wall.ifc, infra-bridge.ifc) are also referenced from an | ||
| * already-covered extension (apps/viewer/src/components/mcp/McpPlayground.tsx). | ||
| * Adding `.rs` was tried and rejected: Rust test fixtures reuse generic | ||
| * basenames like "manifest.json" for their own unrelated corpora, so a | ||
| * basename-only substring search over ~800 .rs files pulls in matches that | ||
| * have nothing to do with apps/viewer/public — noise without closing the | ||
| * gap, since the two real .rs references above are already covered | ||
| * elsewhere. If that TSX reference is ever removed, this exclusion needs | ||
| * re-checking. | ||
| */ | ||
| export const TEXT_EXTENSIONS = new Set([ | ||
| '.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs', '.mts', '.cts', | ||
| '.json', '.html', '.htm', '.css', '.scss', | ||
| '.md', '.mdx', '.yml', '.yaml', '.txt', '.xml', '.toml', | ||
| ]); | ||
|
|
||
| /** | ||
| * @param {object} args | ||
| * @param {string[]} args.assetPaths - paths relative to the scanned | ||
| * directory, e.g. "favicon.ico", "oauth/bcf/callback.html". | ||
| * @param {{path: string, content: string}[]} args.corpusFiles - every other | ||
| * tracked text file in the repo, `path` repo-relative, for substring search. | ||
| * @param {Iterable<string>} args.allowlist - scan-relative asset paths that | ||
| * are exempt (convention-fetched: favicon.ico, robots.txt, etc). | ||
| * @returns {{ unreferenced: string[], allowlisted: string[] }} | ||
| */ | ||
| export function findUnreferencedAssets({ assetPaths, corpusFiles, allowlist }) { | ||
| const allowSet = new Set(allowlist); | ||
| const unreferenced = []; | ||
| const allowlisted = []; | ||
|
|
||
| for (const relPath of assetPaths) { | ||
| const basename = relPath.split('/').pop(); | ||
| const candidates = [basename, `/${relPath}`, relPath]; | ||
| const referenced = corpusFiles.some((f) => candidates.some((c) => f.content.includes(c))); | ||
| if (referenced) continue; | ||
| if (allowSet.has(relPath)) { | ||
| allowlisted.push(relPath); | ||
| } else { | ||
| unreferenced.push(relPath); | ||
| } | ||
| } | ||
|
|
||
| return { unreferenced, allowlisted }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude each candidate asset from its own corpus.
corpusFilesincludes every tracked text file, including the candidate underapps/viewer/public. An unreferenced text asset such asfoo.txtthat containsfoo.txtwill therefore mark itself as referenced and make the gate pass. Keep references from other public assets, but exclude only the current asset when evaluating it.🤖 Prompt for AI Agents