Icon pipeline → public/, delete static/, and run vitest in CI - #236
Merged
Conversation
`pnpm icons:generate` wrote into `static/icons` and `pnpm icons:install` read
from there, but Next only serves `public/`. Regenerating icons updated a tree
nobody serves while the served one silently went stale — which is how the two
directories drifted apart in the first place.
- generate-icons.js reads ./public/favicon.svg and writes ./public/icons.
- It now emits every icon the app references, not a subset: the android-chrome-*
family (which is what public/manifest.json actually points at), the favicon-NxN
set, and the Windows mstile tiles including the non-square 310x150. Previously
regenerating refreshed the apple-touch icons and four icon-* sizes while
leaving the icons Chromium installs with untouched.
- install-desktop-icons.sh copies from public/. Its 16px and 32px fallbacks
looked for static/favicon-{16,32}.png, which never existed, so both branches
were dead; they now use public/icons/favicon-{16x16,32x32}.png.
- Dropped the `needsSolidBackground` branch. `background` only paints the
letterbox that `fit: 'contain'` adds, and favicon.svg is square, so it painted
nothing — the committed icons have always had transparent pixels. Making the
manifest's maskable 192/512 icons genuinely opaque needs `.flatten()` plus
safe-zone padding, which changes how the installed icon looks, so it is left
as a deliberate design decision rather than folded in here.
Running the generator now rewrites exactly the 38 PNGs already in public/icons
and creates no new files, so the script and the tree agree. Regenerated binaries
are not committed — this change is tooling only, no icon artwork changes.
tests/pwa-installability.test.js grows five checks tying the generator to the
manifest and layout: every icon either references must be one the generator
emits, and no tooling may point at static/ again. That is what caught the
missing mstile tiles.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
static/ was a near-exact copy of public/ left behind by the Next.js migration. Next only serves public/, so every one of these 56 files was dead weight — and a trap, since editing static/manifest.json looks like it should do something. Verified before removing: every file under static/ has a counterpart in public/, including .well-known/security.txt (which prod serves 200). The icon tooling that genuinely did read static/ was retargeted at public/ in the previous commit, so nothing references it any more. Also fixed while here: - public/qryptchat.desktop hardcoded `Icon=/home/ettinger/src/qrypt.chat/qryptochat-web/static/qryptchat.png`, an absolute path into a developer's home directory (with a typo in the folder name), so the installed desktop launcher had no icon for anyone else. The installer registers the icon in the hicolor theme, so the entry just needs the theme name: `Icon=qryptchat`. - PWA_VIDEO_PLAYBACK_GUIDE.md told readers to edit static/manifest.json. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI ran CodeQL, Semgrep, npm-audit and gitleaks but never vitest, so nobody noticed the suite could not start at all: vitest.config.js failed to load because @vitejs/plugin-react@6 peers vite ^8 while vitest 4 ships vite 7. That was fixed in #235; this wires the suite into CI so it stays runnable. Triaging all 139 test files in isolation gave 72 passing, 66 failing and 1 hanging. Two mechanical problems accounted for a third of the breakage: - 24 files did `import { describe, it } from 'mocha'` while the repo runs vitest, so those bindings were undefined and the file died on "Cannot read properties of undefined (reading 'describe')". Removed the import (vitest already sets globals: true) and renamed mocha's before/after to vitest's beforeAll/afterAll. - jsdom ships no IndexedDB, but the app stores private keys there, so every test touching key storage hit "indexedDB is not defined". tests/setup.js now imports fake-indexeddb/auto. That took it to 80 passing / 58 failing / 1 hanging. The remaining 59 are listed in tests/quarantine.js and excluded from the default suite: 19 have genuinely failing assertions, 13 import SvelteKit paths the migration deleted, 12 need a live Supabase, 5 use undefined globals, 1 has a parse error, 9 are assorted. tests/pwa-session-integration.test.js hangs rather than fails, which is what made `pnpm test` unusable. They are quarantined rather than deleted because each still documents intended behaviour worth porting, and a permanently red build gets ignored. Work them off with `pnpm test:quarantined`; deleting a line from the list is how a file rejoins CI. tests/QUARANTINE.md explains the categories and the fix for each. CI now runs `pnpm test:ci`: 80 files, 469 tests, ~33s, green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
ThreatCrush Security Scan96 finding(s) HIGH/CRITICAL: 3 | MEDIUM: 17 | LOW: 76
…and 46 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #235. Three commits, reviewable separately.
1.
fix(icons): point the icon pipeline atpublic/This is the one that actually mattered, and it's why I didn't just delete
static/as I'd originally suggested —static/was still live:pnpm icons:generatewrote intostatic/iconspnpm icons:installcopied out ofstatic/iconsBut Next only serves
public/. So regenerating icons updated a tree nobody serves while the served one silently went stale — which is how the two directories drifted apart to begin with, and a live footgun for the PWA icons fixed in #235.Both scripts now read and write
public/. The generator also emits every icon the app references, not a subset: previously it produced the apple-touch icons and fouricon-*sizes while leaving theandroid-chrome-*family — the onesmanifest.jsonactually points at — untouched. Added those plus thefavicon-NxNset and the Windowsmstiletiles (including the non-square 310x150).Running it now rewrites exactly the 38 PNGs already in
public/iconsand creates no new files, so script and tree agree. No regenerated binaries are committed — this is tooling only, no artwork changes.Also dropped the
needsSolidBackgroundbranch:backgroundonly paints the letterboxfit: 'contain'adds, andfavicon.svgis square, so it painted nothing. The committed icons have always had transparent pixels. Making the manifest's maskable 192/512 icons genuinely opaque needs.flatten()plus safe-zone padding, which changes how the installed icon looks — a design decision, not something to fold in silently.Five new tests tie the generator to the manifest and layout: every icon either references must be one the generator emits, and no tooling may point at
static/again. That's what caught the missing mstile tiles.2.
chore: deletestatic/Now safe, since nothing references it. Verified every one of the 56 files has a
public/counterpart — including.well-known/security.txt, which prod serves 200.Also fixed:
public/qryptchat.desktophardcodedIcon=/home/ettinger/src/qrypt.chat/qryptochat-web/static/qryptchat.png— an absolute path into a developer's home directory, with a typo in the folder name, so the installed desktop launcher had no icon for anybody else. The installer registers the icon in the hicolor theme, so the entry just needsIcon=qryptchat.3.
ci: run vitestCI ran CodeQL, Semgrep, npm-audit and gitleaks but never vitest — which is how the broken
vitest.config.js(fixed in #235) went unnoticed.I triaged all 139 test files in isolation: 72 passing, 66 failing, 1 hanging. Two mechanical causes covered a third of it:
import { describe, it } from 'mocha'while the repo runs vitest — those bindings come backundefinedand the file dies onCannot read properties of undefined (reading 'describe'). Removed the import (globals: trueis already set) and renamed mocha'sbefore/aftertobeforeAll/afterAll.ReferenceError: indexedDB is not defined.tests/setup.jsnow importsfake-indexeddb/auto.That got it to 80 passing / 58 failing / 1 hanging. The remaining 59 are listed in
tests/quarantine.jsand excluded from the default suite:+server.js,$lib/*)sinon)await)Quarantined rather than deleted: each still documents behaviour worth porting, and a permanently red build gets ignored.
pnpm test:quarantinedruns just those so the backlog can be worked off; deleting a line is how a file rejoins CI.tests/QUARANTINE.mdhas the breakdown and the fix for each category.CI runs
pnpm test:ci: 80 files, 469 tests, ~33s, green.Note
tests/pwa-session-integration.test.jshangs rather than fails — that's what madepnpm testunusable, and worth fixing early.Verification
pnpm test:ci— 80 files / 469 tests passingpnpm test:quarantined— runs the 59, fails as expectednode scripts/generate-icons.js— 38 PNGs intopublic/icons, no new filesstatic/file confirmed to exist inpublic/before deletion🤖 Generated with Claude Code