Skip to content

chore: add fallow config and fix high-signal findings#938

Merged
jrusso1020 merged 2 commits into
mainfrom
chore/fallow-config-and-cleanup
May 18, 2026
Merged

chore: add fallow config and fix high-signal findings#938
jrusso1020 merged 2 commits into
mainfrom
chore/fallow-config-and-cleanup

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 18, 2026

What

Add a .fallowrc.jsonc and fix the genuine code-quality issues fallow surfaces. Drops dead-code findings from 601 → 276 (45% noise reduction from config, the rest are real signal that's either fixed here or listed as deferred follow-ups).

Why

npx fallow --summary on main was reporting 601 dead-code issues, but ~half were false positives: docs .mdx files flagged as unused, vitest/bun-test files not registered as entry points, worker entry points loaded via file path invisible to static analysis, deps consumed dynamically (tsup external, peer/static-file in tests, readFileSync reads of @fontsource/*). Without config, fallow can't be a useful signal; with config, the remaining findings are real.

How

Config (.fallowrc.jsonc)

  • Declare test/runtime/worker/script entry points that fallow's plugin heuristics miss
  • Ignore docs, test fixtures, skill test-corpora, registry, examples, CI fixtures
  • Ignore deps that are dynamically referenced (puppeteer-core, esbuild, giget, gsap, happy-dom, ffmpeg-static, ffprobe-static, all @fontsource/*, workspace siblings)
  • ignoreExports for intentional namespace barrels (per-namespace ML manager exports; CLI per-command examples convention from CLAUDE.md; isPathInside in fileServer.ts which is test-only and has different symlink semantics from the canonical one in utils/paths.ts)

Inline build:fonts blob extraction
packages/{cli,producer}/package.json had node -e "..." blobs containing {...} that fallow mis-parsed as glob alternate groups, emitting 4 warnings and skipping the real entry. Moved each into a dedicated scripts/build-fonts.mjs.

Duplicate-export fixes

  • FileIcon in studio/SystemIcons.tsx — removed the dead alias; FileTreeIcons.tsx has the real, used one
  • ValidationResultgsapParser.ts had an identical-shape duplicate of core.types.ts. Dropped the duplicate; both parsers now import from core.types

Circular dep
manualEditsDom.ts re-exported three clearStudio* functions from manualEditsSnapshot.ts, which imports four helpers from manualEditsDom.ts — a back-edge cycle. Moved the re-export to manualEdits.ts (the package-public barrel) where the rest of the snapshot re-exports already live.

Unused-dep cleanup

  • studio: drop motion (no imports anywhere) and codemirror (umbrella package; @codemirror/* sub-packages are used directly)
  • cli: drop mime-types (zero imports), its now-orphan src/utils/mime.ts (hardcoded mime table that didn't even use the package), and its tsup external entry

Test plan

  • bun run typecheck clean across core, cli, producer, studio
  • bunx oxlint + bunx oxfmt --check clean on changed files
  • bunx vitest run packages/studio/src/components/editor/manualEdits.test.ts — 18/18 pass (verifies the cycle break doesn't break behavior)
  • bunx vitest run packages/core/src/parsers — 69/69 pass (gsapParser + htmlParser tests verify the ValidationResult consolidation)
  • bunx vitest run packages/studio — 505/505 pass
  • bunx vitest run packages/cli — 332/332 pass
  • node scripts/build-fonts.mjs in both packages/cli and packages/producer runs successfully

Deferred follow-ups

Real findings that need their own PRs:

  • 8 circular deps in producer/services/render/stages/renderOrchestratorcaptureHdr* / captureStage / extractVideosStage form a hub cycle. Real refactor.
  • ~14 unused files in producer/src/services/ (audioExtractor, audioMixer, browserManager, chunkEncoder, compilationRunner, parallelCoordinator, screenshotService, streamingEncoder, videoFrameExtractor) — most are thin re-export shims to @hyperframes/engine that aren't in the public exports map. Need to confirm no deep-import consumers before deletion.
  • waveform.ts complexity hotspot — fallow's start with... suggestion. Separate audit.

Configure fallow via .fallowrc.jsonc so its analysis reflects this repo's
real entry surface, then fix the genuine issues it found.

Fallow noise reduction (601 → 276 dead-code findings):
- Ignore docs/, test fixtures, skill test-corpora, registry/, examples/
- Declare worker entry points loaded dynamically by file path
  (pngDecodeBlitWorker.ts, shaderTransitionWorker.ts)
- Declare runtime IIFE entry (core/src/runtime/entry.ts) built outside the
  import graph by build-hyperframes-runtime-artifact.ts
- Declare bun:test files in producer + aws-lambda as test entries
- Ignore dynamically-resolved deps: tsup external (puppeteer-core, esbuild,
  giget), peer/static-file (gsap in player perf tests), workspace deps
  hoisted by bun (happy-dom, @hyperframes/*), and @fontsource/* packages
  read via readFileSync in generate-font-data.ts

Extract inline build:fonts scripts:
- packages/{cli,producer}/package.json had multi-line `node -e ...` blobs
  containing braces that fallow mis-parsed as glob alternate groups. Moved
  to dedicated build-fonts.mjs scripts.

Fix duplicate exports:
- Remove dead FileIcon alias in studio/SystemIcons.tsx (FileTreeIcons.tsx
  has the real, used one)
- Consolidate ValidationResult: drop the identical duplicate in
  gsapParser.ts; both parsers now import from core.types
- Suppress intentional namespace patterns (per-namespace ML manager
  exports; CLI per-command 'examples' convention; fileServer.ts test-only
  isPathInside which has different symlink semantics from utils/paths.ts)

Break circular dep (studio/components/editor):
- manualEditsDom.ts re-exported clearStudioPathOffset / clearStudioRotation
  / clearStudioBoxSize from manualEditsSnapshot.ts, which imports four
  helpers from manualEditsDom.ts — back-edge cycle
- Re-export moved to manualEdits.ts (the package-public barrel) where the
  rest of the snapshot re-exports already live; underlying files now form
  a clean DAG

Remove genuinely unused deps:
- studio: motion (no imports anywhere), codemirror (umbrella package; the
  @codemirror/* sub-packages are used directly)
- cli: mime-types (plus its only consumer src/utils/mime.ts, which was a
  hardcoded mime table that didn't use the package), and its now-stale
  tsup external entry

Verified: typecheck across core/cli/producer/studio is clean, oxlint
+ oxfmt pass, manualEdits.test.ts (18 tests) and core parser tests (69
tests) still pass.

Deferred follow-ups (real findings, separate PRs):
- 8 circular deps in producer/services/render/stages/ — renderOrchestrator
  ↔ captureHdr* / captureStage / extractVideosStage form a hub cycle
- ~14 unused files in producer/src/services/ that look like dead
  re-export shims to @hyperframes/engine, but aren't in the public
  exports map — need to confirm no deep-import consumers before deletion
- waveform.ts complexity hotspot
Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-scoped PR. Every change is justified and the config is well-documented.

Verified:

  • FileIcon in SystemIcons.tsx is dead — FileTreeNodes.tsx imports from FileTreeIcons.tsx, not SystemIcons
  • mime-types has zero imports across cli/src
  • codemirror umbrella and motion have zero imports in studio/src — sub-packages (@codemirror/*) are used directly
  • ValidationResult consolidation: core.types.ts is the canonical location, both parsers now import from there
  • Circular dep fix: moving the clearStudio* re-exports from manualEditsDom.ts to the manualEdits.ts barrel breaks the back-edge without changing the public API
  • build-fonts.mjs extraction: eliminates the fallow glob-parsing false positives from inline node -e blobs

The deferred follow-ups (8 circular deps in producer/services/render/stages, ~14 unused shim files, waveform complexity) are the right call for separate PRs.

CI green except one regression shard still in progress — no code changes that would affect render output.

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, well-scoped cleanup. Config is explained (not bulk-allowlisted), each ignore has a one-line rationale, and the structural fixes (cycle break, duplicate-type merge, dead-code drops) are individually verifiable. Public-API surfaces preserved. Recommend ship.

Calibrated strengths

  • .fallowrc.jsonc ignore rationales are concrete and load-bearing — e.g. isPathInside carries the "different symlink semantics from utils/paths.ts" note, so the next reader knows the duplication is intentional rather than a latent dedup target. This is exactly the right discipline for a tool config — it's documentation for the next person who hits a fallow flag.
  • manualEdits.ts:42-50 — moving the clearStudio* re-export off the back-edge in manualEditsDom.ts:464 and onto the package-public barrel is the structurally correct cycle break. Verified the only external consumer (hooks/useDomEditCommits.ts:11-15) goes through the ./manualEdits barrel, so the public surface is unchanged. manualEdits.test.ts also imports via the barrel — tests cover the new path.
  • ValidationResult consolidation: the duplicate at gsapParser.ts:347 is dropped, both parsers now import from core.types, and core/index.ts:20 continues to re-export the canonical one. No deep-import consumers (from "@hyperframes/core/parsers/gsapParser") were depending on the type — verified across the workspace.
  • The node -escripts/build-fonts.mjs extraction is the right call. Even setting aside fallow's glob-parser issue, the inline blob was unreadable and unreviewable; a dedicated 14-line script with existsSync + execSync is much easier to maintain. Symmetric move in both cli and producer.

Findings

nit — bun.lock carries a stray aws-lambda version bump (0.0.1 → 0.6.22). Not in the PR description, and packages/aws-lambda/package.json doesn't appear in the diff. Looks like an unrelated lockfile resync from running bun install against a workspace whose version fields had drifted on main. Not load-bearing (aws-lambda isn't published as 0.6.x afaict), but worth a sentence in the description so the next person reviewing the lockfile delta isn't confused. Same goes for the synchronized 0.6.15 → 0.6.22 bumps across every workspace package.

nit — mime-db regressed 1.54.0 → 1.52.0 in the lockfile after dropping the direct mime-types dep. The remaining aws-cdk-lib/mime-types chain now wins resolution at the older version. No functional impact (only consumed transitively by aws-cdk-lib), but if some downstream tool reads top-level mime-db it'll see the older version. Worth a glance, but not a blocker.

nit — fallow worker-entry declarations point at file paths that don't exist on main yet (packages/producer/src/services/pngDecodeBlitWorker.ts, shaderTransitionWorker.ts). Verified the typecheck CI is green on this PR, so either (a) those files exist on the branch I haven't traversed, or (b) fallow tolerates missing entries gracefully. Just flagging that adding entries-that-don't-yet-exist sets up a foot-gun if a future rename silently drops them off the entry list — a comment near those two lines noting the companion *Pool.ts files would prevent future drift.

nit — deferred follow-up bullet for the 8 producer/services circular deps is the right move, but consider opening a tracking issue before this merges. "Real refactor, listed in PR body" tends to fall off when the PR merges and the body scrolls off; an issue keeps it indexed.

CI (verbatim, fresh)

{"failed":[],"pending":["regression-shards (shard-1, ...)","regression-shards (shard-2, ...)","regression-shards (shard-3, ...)","regression-shards (shard-4, ...)","regression-shards (shard-5, ...)","regression-shards (shard-6, ...)","regression-shards (shard-7, ...)","regression-shards (shard-8, ...)"]}

All required & non-shard checks green (Build, Typecheck, Lint, Test, CLI smoke, runtime contract, Format, Preflight, file size, semantic title, perf gates, Windows render, preview-regression, CodeQL). 8 regression shards still in progress; nothing failed.

Verdict

Verdict: APPROVE
Reasoning: Every non-trivial deletion verifies — FileIcon alias isn't consumed outside its file (the FileTree.tsx FileIcon is a separate local symbol, not the same export), mime-types / motion / codemirror umbrella have zero imports, and the ValidationResult + cycle-break refactors preserve the public surface and pass typecheck. Config rationales are the right level of detail. The lockfile drift is a noise item, not a correctness one.

Review by Vai

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-stamp converting the prior COMMENT to a formal APPROVE — content of the review body still applies.

Verified deletions

Every removed symbol grep-verified across workspace including registry/, examples/, docs/:

  • FileIcon — only out-of-file ref is a separate local symbol in FileTree.tsx. Clean.
  • mime-types, motion, codemirror packages — zero imports across the workspace. Clean.
  • MIME_TYPES in cli/utils/mime.ts — file-local only; other files have their own MIME tables. Clean.

Cycle break

useDomEditCommits.ts:11 (only external consumer of clearStudio*) goes through the ./manualEdits barrel, which the PR updates to re-export directly from manualEditsSnapshot. Public surface preserved — no adopter break.

Findings

nit: lockfile churn — every workspace's 0.6.15 → 0.6.22 bump plus a stray aws-lambda 0.0.1 → 0.6.22 bump aren't in the PR body. Either intentional (version sync) or accidental. Worth a line in the description either way.

nit: the fallow tool itself is a nice addition for the team's static-analysis bar. If you intend to keep using it, a bunx fallow pre-commit lefthook or CI step would lock in the cleanup discipline.

Verdict: APPROVE.

Re-stamp by Vai

@jrusso1020 jrusso1020 merged commit 1edea8a into main May 18, 2026
36 of 45 checks passed
@jrusso1020 jrusso1020 deleted the chore/fallow-config-and-cleanup branch May 18, 2026 20:52
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.

3 participants