Skip to content

Remove generated bloat and simplify the project - #268

Open
byalex33 wants to merge 2 commits into
FactoryGameFan:wormeyman-space-age-supportfrom
byalex33:cleanup/project-simplification
Open

Remove generated bloat and simplify the project#268
byalex33 wants to merge 2 commits into
FactoryGameFan:wormeyman-space-age-supportfrom
byalex33:cleanup/project-simplification

Conversation

@byalex33

@byalex33 byalex33 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Been working on this for a few hours — hope it is not too much of a PR 😅

What changed

Removed generated and obsolete files

  • Removed 5,718 unreferenced numbered exporter artifacts (* 2.basis, * 3.basis, and numbered metadata), cutting 291.55 MiB from the checkout.
  • Added ignore rules so those duplicate exporter artifacts are not recommitted.
  • Removed nine completed implementation plans (7,140 lines of historical task notes).
  • Removed the obsolete Pages-era functions/corsproxy.js; the live proxy remains in the Cloudflare Worker.
  • Removed the unused worker placeholder public/index.html.

Simplified application code and dependencies

  • Routed assemblers, beacons, and mining drills through the existing TempEditor and removed three redundant editor classes.
  • Replaced file-saver and its typings with the browser's native object-URL/download-anchor APIs.
  • Replaced the single utility-types use with a local FunctionKeys type.
  • Updated the lockfile; this removes three installed packages.

Simplified tooling

  • Removed the redundant type-check baseline gate, its tests, baseline file, script, CI step, and Vite test project. vp check already enforces strict package-aware type checking at zero errors.
  • Consolidated the duplicated Rust sprite-compression implementations used by downloaded and local Factorio data.
  • Added one shared Factorio oracle probe runner for isolated mod setup, execution, and dump handling.
  • Migrated 17 probes to that runner, removing roughly 700 repeated lines.
  • Added a small runnable self-check for the shared runner.

Made project guidance current

  • Reduced CI from 428 lines to 217 while preserving the checks, Rust builds, four Playwright shards, caching, and deployment gates.
  • Updated contributing/toolchain instructions for Vite+ 0.2.9 and npm 12.
  • Removed stale strict-migration and deleted-file notes from live configuration.

Result

  • 5,767 files changed
  • 423 insertions, 8,740 deletions
  • 291.55 MiB of duplicate generated output removed
  • No new runtime dependency

Verification

  • vp check . — formatting, lint, and type checks pass (176 files)
  • vp test — 172/172 unit tests pass
  • npx playwright test — 198/198 browser tests pass
  • production website build passes (3,061 assets copied)
  • cargo check --locked passes
  • cargo clippy --locked -- -D warnings passes
  • cargo fmt --check passes
  • oracle runner self-check and syntax checks for every oracle script pass

Note

The CLAUDE.md rewrite is now isolated in #277 for independent review.

vp install still reports the repository's existing npm audit state (two moderate advisories and one high). I did not mix an unrelated automated dependency upgrade into this cleanup PR.

@wormeyman

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown

Claude finished @wormeyman's task in 4m 50s —— View job


Review

  • Gather context (PR description, changed files list)
  • Inspect diff against origin/wormeyman-space-age-support (reviewed commit 28697b2 against its parent e5600f6, which matches the file-level diff)
  • Review removed generated files / gitignore rules
  • Review editor consolidation (TempEditor, removed editor classes, factory.ts)
  • Review file-saver replacement and utility-types replacement
  • Review CI workflow simplification and type-check gate removal
  • Review CLAUDE.md rewrite for accuracy
  • Review oracle probe runner consolidation
  • Post final review

Overall this is a well-executed cleanup, and the mechanical parts check out. The one thing I'd want a maintainer to weigh in on before merging is the CLAUDE.md rewrite, since it's the highest-risk change here.

Verified as correct

  • Exporter artifact removal: all 5,718 deleted files under packages/exporter/data/output end in 2.basis/ 3.basis/metadata 2.json/metadata 3.json, matching the new .gitignore patterns exactly. No non-numbered asset was touched.
  • packages/website/src/index.ts: the saveBlob replacement for file-saver is a correct, standard object-URL/anchor pattern (creates URL, clicks, revokes on next tick). Minor: it doesn't append the anchor to the DOM before .click(), which some older WebKit builds require — almost certainly fine for this project's supported/desktop-only editing target, just flagging it since it's the one behavioral (not just mechanical) change in the diff.
  • packages/editor/src/UI/editors/factory.ts / TempEditor: I checked the deleted BeaconEditor, MachineEditor, and MiningEditor against TempEditor's existing conditional logic (acceptedRecipes.length > 0 for the recipe row, moduleSlots !== 0 for the modules row, with the i += 38 offset). The layout coordinates it produces for beacon/machine/mining-drill are identical to what the three deleted classes hard-coded. This consolidation looks behavior-preserving.
  • TextInput.ts: the local FunctionKeys<T> type is an exact structural match for utility-types' version. typescript/no-explicit-any is already off in vite.config.ts, so the any[] in the mapped type won't trip lint.
  • .github/workflows/ci.yml: diffed line-by-line — the only functional removal is the Type-check gate step; everything else stripped was explanatory comments. This matches the PR description (baseline gate is redundant now that strict: true is enforced at 0 via vp check). package.json, vite.config.ts, and tsconfig.json were updated consistently (script, vitest project, and baseline-migration comment all removed together).
  • packages/exporter/src/setup.rs: the new compress_sprites() helper is a faithful extraction of the two previously-duplicated blocks in extract() and extract_local() — no logic changes, just deduplication.
  • Oracle probe runner: spot-checked probe-elevated-rail-collision.mjs against its pre-PR version — the extracted prepareProbe/runProbe in factorio-probe.mjs reproduce the inlined temp-dir/mod-dir/config.ini/spawn logic exactly, including the info.json field ordering (name still lands correctly via the spread). factorio-probe.test.mjs mocks spawn and asserts on the dump path, temp dir, and info.json — a real regression test, not just a syntax check. It's not wired into vp test or CI (matches the PR's description of it as a standalone "runnable self-check" rather than a suite addition) — worth confirming that's intentional.
  • Dead file removals: confirmed functions/corsproxy.js and packages/worker/public/index.html have no remaining references anywhere in the tree (the live proxy route in packages/website/vite.config.js already targets the deployed Worker, and the Worker serves packages/website/dist via ASSETS, not packages/worker/public).
  • package-lock.json: removal is exactly the three packages (file-saver, @types/file-saver, utility-types) with no incidental changes.
  • CONTRIBUTING.md: the npm ^11^12 correction matches what package.json's devEngines actually declares (it was already ^12; CONTRIBUTING.md was stale). The vite.plus install snippet is also fixed to put VP_VERSION/VP_HOME before bash instead of before the curl | bash pipe — this was a real bug (the env vars never reached the installer in the old form), and CLAUDE.md's own "Vite+ Toolchain" section already documents why.
  • docs/superpowers/plans/* deletion: these are completed implementation checklists; docs/superpowers/specs/ (design docs) is untouched, and nothing else in the repo references the deleted plan files.

Worth a second look: the CLAUDE.md rewrite

This PR replaces CLAUDE.md's ~996 lines with 197. Some of that is genuinely dead weight (the file had accreted like a diary). But the cut is much deeper than "diary cleanup" — entire categories of content are gone with nothing moved elsewhere in the repo:

  • The whole "Version Constraints" section — why typed-factorio is held on 3.x (dist-tag mapping to the Factorio version this project targets), why vite-plus is pinned in five places including a sha256 that can rotate independently of the version, the Renovate fork-processing gotcha (forkProcessing silently skipping the whole repo), and the undici/npm audit situation (blocked upstream via exact pins, npm audit fix actively makes it worse). None of this is derivable by reading code — it's the record of specific investigations and incidents (dates, npm view output, measured behavior).
  • Nearly all of the "Asking the real game" methodology — the specific measured numbers (snap radii, zoom step derivation, rail placement exceptions), and more importantly the method notes ("a control has to be able to fail while the hypothesis holds", "ask what a limit is before comparing it to a number", etc.) that were clearly written so the next probe wouldn't repeat the same mistakes.
  • The per-spec rationale in the Playwright section — why each test exists, what bug it was written to catch, what mutation-testing found. The new version has three short paragraphs covering the whole test suite.
  • The fork-identity narrative (why the logo/assets are regenerated, the relationship to upstream teoxoy, the GitHub "fork" detach status) is also gone from the new file entirely.

None of this was moved into docs/ — I checked, and the only docs/ changes in this PR are the plan deletions. If a future contributor (human or agent) hits the vite-plus pin or the undici audit finding again, they'll redo the investigation from scratch, and there's a real risk of them "fixing" something that's actually intentionally held.

This might be exactly what the maintainer wants — trading institutional memory for a leaner, more approachable guide, on the assumption that git history/issues hold the detail if truly needed. But given how deliberately that content was built up (it reads like each paragraph exists because something went wrong once), I'd flag it for explicit sign-off rather than approve it as a pure cleanup. A middle path would be keeping CLAUDE.md lean as this PR does, but moving the Version Constraints and oracle-method-notes content into a docs/ reference file instead of deleting it outright.

Fix this →

@wormeyman

Copy link
Copy Markdown
Collaborator

@byalex33 Let's put the Claude markdown file into its own pull request and then we can ship everything else in this pull request.

@byalex33

Copy link
Copy Markdown
Contributor Author

Done — restored CLAUDE.md here and moved its rewrite to #277 for independent review.

@wormeyman

Copy link
Copy Markdown
Collaborator

Two blockers, both small.

  1. The deleted gate vitest project globbed scripts/**/*.test.mjs, which collected two files rather than one. scripts/localpreview.test.mjs survives with no collector, so its 6 tests stop running. That includes isPortFree is false for an IPv6-only listener, which guards the macOS case where Vite binds ::1 only and localpreview reports 8080 free. Adding scripts/**/*.test.mjs to the surviving unit project's include fixes it.

  2. tools/oracle/factorio-probe.test.mjs is collected by nothing, and it has no import { test } from 'vite-plus/test', so it would register zero tests even if it were collected.

Everything risky I checked holds up. The 5,716 sprite deletions match data.json's 3,060 references exactly, with no required file lost. The CI change only removes the gate step. The three editor classes render identically through TempEditor, because beacon and electric-mining-drill are not crafting machines, so acceptedRecipes is empty and the offset stays 0. The Rust consolidation is byte-identical after one identifier rename.

Two smaller things. The shared runner drops the factorio_version vs binary line from its no-dump error, while the migrated probe's own comment still warns about that exact silent failure. And saveBlob revokes the object URL on the next macrotask where file-saver waits 40 seconds, on a path with no test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants