Quality floor: type-aware lint, import validation, Electron hardening - #83
Merged
Conversation
Flat config with typescript-eslint recommendedTypeChecked plus react-hooks, split across three surfaces: type-aware rules for src/, plain recommended for the electron/ Node scripts and root configs. Prettier config last so nothing fights the formatter. CI gains a Lint step between Typecheck and Tests. The notable catches, beyond the mechanical unused-import sweep: - LightEditor called useCallback below an early return; if lightInfo flipped null <-> non-null while mounted, React would throw on hook order. Hook hoisted above the guard. - handleCycleEntityRotationCCW was missing selectedPaletteItem in its deps; the CW twin had it. Stale closure, decal rotation only. - buildYaml was missing state.decalsDirty in deps, so a save right after a decal-only edit could serialize without the decal rewrite. - Every async click handler / fire-and-forget promise now either catches or is explicitly void with the reasoning commented. - onFocusGrid/handleFocusGrid: dead since the original open-sourcing commit (prop wired, no UI trigger ever existed). Removed; git keeps the fitBounds handler if a focus-grid feature lands later. no-explicit-any and the v7 React Compiler diagnostics stay warnings: the any debt sits at the YAML ingest boundary where Zod is the real fix, and the compiler rules flag documented-deliberate patterns.
importMap parsed straight into `as any` and trusted the traversal: point it at a prototype file or half a merge conflict and it died three calls deep with a TypeError. There is now a Zod schema (mapSchema.ts) between yaml.load and the traversal. The schema is deliberately the thinnest thing that guarantees what the traversal assumes: doc is a mapping, entities is an array of groups, uids are ints, components are mappings. Fields type-check only when present, unknown keys pass through, component bodies stay opaque. The round-trip contract is byte-exact and players hand-edit these files, so anything the schema rejected would become an unopenable file; when in doubt it stays loose. Parity sweep against the Triad corpus: no change (the one pre-existing failure, test_teg.yml, is a duplicated mapping key that js-yaml refuses upstream of the schema; the engine parser is last-wins).
One bad prototype in a stranger fork could throw during render and whitescreen the whole editor, unsaved work included, with nothing but the devtools console to explain it. The boundary catches render-phase crashes, shows the error with a trimmed stack, and offers reload (which lands on the fork selector recent-files list) plus a link to the issue tracker.
The round-trip exporter dispatches on key presence ('maps' in doc), so
missing-vs-undefined is a real distinction in this codebase and worth
having the compiler police. Fields that are genuinely assigned
undefined-as-a-value are widened to `?: T | undefined`; no conditional
spreads or guards, because changing which keys exist on an object is a
behavior change here.
- sandbox: true. The preload only touches contextBridge/ipcRenderer,
so it runs unchanged in the OS sandbox; a compromised renderer can
no longer reach Node.
- openExternal now gated to http(s). Deny-and-open was already the
shape, but any URL reaching it went to the OS shell, file:// and
custom protocol handlers included.
- will-navigate handler pins the window to the app origin.
- CSP on the packaged app (served with the HTML from the app://
handler): everything 'self' except inline style attributes (React
style={{}}) and blob:/data: images (sprite thumbnails). Verified no
workers, no eval, no external fetch in src. Dev server stays exempt;
Vite HMR needs its own policy.
The flag surfaces ~750 errors repo-wide; that is a migration, not a config change. scripts/check-strict-indexing.mjs runs tsc with the flag over the whole program and fails CI only for errors inside cleaned directories (a per-directory tsconfig include cannot do this: tsc reports errors across the transitive import graph, so the dirty core would leak into any slice). algorithms/, settings/, validation/ and hooks/ are cleaned and enforced; the dirty-directory error counts are recorded in the script for whoever picks up the next slice. state/ is the highest-value target (155 errors, and it is the reducer). Source fixes are `as const` on the 4-neighbor offset tables; test fixes are non-null assertions behind explicit length expectations.
ESLint types the tests without the ratchet flag (tsconfig has it off), so the assertions read as unnecessary there while the ratchet run requires them. Optional chaining satisfies both type universes and fails the assertion cleanly instead of throwing if the index ever comes back empty.
The script it references was removed when the static-deployment story was carved out (2efe4e0) and public/resources/ no longer exists; nothing in the tree generates it.
- README: CI gauntlet now lists lint and the strict-indexing ratchet; Node floor raised to 20.19+ (ESLint 10 requirement; CI runs 22). - import-export.md: the Zod structural-shell gate is now part of the import pipeline diagram, with a note on why it stays loose. - ui-components.md: ErrorBoundary reference section. - CHANGELOG: Unreleased entries for the crash screen, the readable invalid-file import error, the CCW decal-rotation stale closure, and the stale decal-state save.
This was referenced Jul 28, 2026
Merged main in (PR #81, chrome focus-steal fix) so the branch lints the same tree CI does; the new test file had one `as string` on a value already typed string, which the type-aware lint flags.
4 tasks
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.
About
This raises the baseline for catching problems before they ship, and fixes everything the new tooling found on its first pass.
The centerpiece is ESLint with typescript-eslint's type-aware rules plus react-hooks. It wasn't just plumbing; day one it caught a conditional hook call in LightEditor that would crash on hook-order if the light info ever flipped while mounted, a stale closure in the CCW decal rotation handler (the CW twin had the dep, CCW didn't), a missing
decalsDirtydep inbuildYamlso a save right after a decal edit could serialize stale, and about 27 async click handlers and file reads that failed silently. Also removedonFocusGrid, which turned out to be dead wiring inherited from the original open-sourcing commit.On top of that:
importMapnow validates the structural shell of the document with a deliberately thin Zod schema before traversal. Pointing the editor at a prototype file gets a readable status-bar error instead of a TypeError three calls deep. Component bodies stay opaque and unknown keys pass through, so the round-trip contract is untouched.exactOptionalPropertyTypesis on. The exporter genuinely dispatches on key presence ('maps' in doc), so missing-vs-undefined is a distinction worth having the compiler police.noUncheckedIndexedAccessratchet: the flag is ~750 errors repo-wide, soscripts/check-strict-indexing.mjsenforces it only in cleaned directories (algorithms, settings, validation, hooks so far) and CI keeps them clean. Remaining dirty counts are recorded in the script;state/is the next slice worth doing.sandbox: true,openExternalgated to http(s), awill-navigateorigin pin, and a CSP on the packaged app.no-explicit-anystays a warning for now. The 186 remaining are almost all the YAML ingest boundary, and the honest fix there is extending schema coverage, not hand-typing casts.Testing
test_teg.ymlhas a duplicated mapping key js-yaml refuses upstream of anything this branch touches. Worth its own issue.sandbox: trueand the CSP. The preload only uses contextBridge/ipcRenderer so it should be sandbox-clean, but anpm run electron:devrun before merge is the right check.Checklist
npm run testpassesnpm run format:checkis clean