Re-architect the scene runtime: decompose god-functions, opt-in DRAFT seams, behavior-first tests - #160
Open
Brad-Edwards wants to merge 28 commits into
Open
Re-architect the scene runtime: decompose god-functions, opt-in DRAFT seams, behavior-first tests#160Brad-Edwards wants to merge 28 commits into
Brad-Edwards wants to merge 28 commits into
Conversation
…ting
Authors write { id, title, create, timeline, cleanup }; every other
SceneModule field defaults via defineScene, which validates the
normalized result against the contract. buildTemplateScene now
delegates to it so scene defaults live in exactly one place.
Mode becomes data, not control flow: audio policy, slice truncation, chrome visibility, bed suppression, the scrub cue gate, and head-scene runner hints were scattered as mode === X branches across scene-loader and workbench-chrome; they now read one frozen profile per mode. Behavior is unchanged, pinned by a snapshot-equivalence test against the prior scattered logic for all eight modes. Collapsing the four runner-hint ternaries into a single ...runnerHints spread dropped runLifecycle below the cognitive-complexity gate; its suppression and backlog row are removed.
…exity gate Split the scene-loader god-functions into cohesive single-responsibility units so both drop under the cognitive-complexity gate and their biome-ignore suppressions are deleted: - src/runtime/scene-loader-ctx.ts: per-navigation audio service, presenter pipe (PUL-F025 master-mute handler), deterministic RNG seed, and the per-occurrence ctx factory (the old buildLoad try/catch body). - src/runtime/scene-loader-guard.ts: the present-mode audio unlock-gate predicate (PUL-F030) and the composition chrome dispatch policy (PUL-F031), as the navigation trust seams. - src/runtime/navigation.ts: a single NAVIGATION_GRAMMAR rule source for the beat/mode grammar, consumed by parseNavigationSearch AND the loader's defense-in-depth validateBeatGrammar/validateModeGrammar re-check (now exported from navigation). The duplicated rule strings are eliminated; the forged-target trust seam is retained. runTarget now delegates to dispatchLifecycleLoad + awaitLoad and folds its two grammar checks into one. buildLoad delegates services construction and run-input assembly, keeping the abort/queue/cleanup- exactly-once lifecycle and stage-attr ordering intact. createSceneLoader, all exported types, the data-pulsar-* attributes, and runtime behavior are unchanged. Both noExcessiveCognitiveComplexity suppressions and their complexity-backlog rows are removed.
…t-mode seam Move PresenterTransportState, presenterAdvance/skip, applyPresenterCommandToMaster, and wirePresenterCommands out of the always-on timeline composition path into src/runtime/presenter-transport.ts. The transport is wired onto the master only when a navigation forwards a presenter controller (mode=present); a non-present navigation never instantiates it. Keep the GSAP composition spine in timeline.ts (composeMasterTimeline, the scene label namespace, assertSceneTimeline, MasterBeat). Factor a single validateLabelTime helper, table-drive the pause-at-beat positionMaster branches, and fold the onMaster/onSegmentChange observability into a SegmentReporter object. Public signatures, data-pulsar-* attributes, and cross-engine timing behavior unchanged.
Decompose the two suppressed audio-engine functions so they fall under the maxAllowedComplexity-15 gate without changing observable behavior: - unlock() delegates its HTML5 and Web Audio fallback branches to the new unlockHtml5Fallback / resumeWebAudioContext module helpers. - play() delegates option validation to validatePlay and per-instance engine output to applyPlayToHandle. The normalizeSources offender was already covered by the hoisted normalizeAudioUrl. Remove all three audio.ts rows from the complexity backlog and drop audio.ts from the complexity-gate policy oracle's expected-suppression list (the documented ratchet path). The audio service public methods, output policies, error families, composition bed routing, cue gate, and master-mute semantics are unchanged; the full suite (2669 tests) stays green.
…-driven play-option checks Replace the per-service `disposed` boolean and its ~14 scattered `if (disposed) return` guards with a single internal AbortController: the navigation signal and an explicit `stopAll()` both abort it, and one abort listener runs the stop+unload teardown exactly once. The disposal gate is now a single AbortSignal. Register the composition audio bed through the same `registerSound` core scene sounds use instead of a bespoke `startBed` that duplicated `engine.createSound`. The reserved, non-kebab `composition audio bed` id and the bed's own-declaration source allowlist are preserved, so the bed stays unreachable through `ctx.audio` and gated against the composition's bed declaration. Fold the four per-option `assertPlayOption*` helpers into a table-driven `assertPlayOptions` while preserving the exact per-option error classes and messages. Behavior is byte-identical: public method signatures, the outputPolicy union, error families, bed routing/suppression, cue gate, master mute, and abort teardown are unchanged. Full suite (2669) green.
The sprite map is a field of SoundDefinition, so validating it inside assertSoundDefinition (the scene-facing load boundary) puts the whole definition payload through one boundary assert and removes the standalone assertSpriteMap helper. Per-entry checks move to assertSpriteEntry so the iteration stays flat. The throws scenes observe for a malformed sprite are unchanged.
… internals Embed each scene's SceneActivation in its plan step at build time, so the lifecycle helpers no longer reconstruct it per call. Collapse the three finalizers (happy-path / aborted-playback / composition-wide) into one finalize() that selects aggregate-vs-reraise from whether onSceneFailed was supplied. Wrap onSceneFailed once at lifecycle-context build (reportFailure) and drop the per-call notifyFailure. Separate the bare mount->compose->run->cleanup engine (runLifecycle) from the scene-failure-isolation decorator (resolveComposition) so the orchestrator is testable without the isolation layer. In scene-navigation, unify the three composition-resolution paths (from-start / scene / index) behind one parameterized index finder, and consolidate buildResolverOptions' conditional spreads into a single strip. Drop the resolver's buildRunOptions for a compact picker (the adapter reads via optional chaining, never 'key' in opts). Add both-path (onSceneFailed supplied / omitted) regression coverage asserting cleanup-exactly-once-per-activation and correct error routing for create-throw, timeline-throw, cleanup-throw, abort-mid-mount, and a repeated scene id where one occurrence fails. Public signatures, data-pulsar-* attributes, error wording, and lifecycle invariants (mount-all, reverse cleanup, scene-failure isolation) are unchanged.
…ifecycle engine
Deliver clause (d) of the composition-resolver-internals phase: the
scene-failure-isolation concern (failure bucket ownership, once-wrapped
onSceneFailed, aggregate-vs-reraise routing) is now a named
withFailureIsolation decorator that resolveComposition is composed from,
rather than inline in resolveComposition. Export the bare engine
(runLifecycle plus buildPlan / buildLifecycleContext) so the orchestrator
is testable bare, and add a bare-engine test suite driving runLifecycle
with a plain collecting reportFailure. Remove the dangling
{@link withFailureIsolation} reference that pointed at a symbol which did
not exist. Behavior, public resolver signatures, data-pulsar-* attributes,
and error wording are unchanged.
Replace the structural fake-DOM types (TemplateDomElement / TemplateDomFactory / TemplateStageElement) and the per-call defensive ctx narrowing (isTemplateCtx / isStageShape / isGsapShape) with real lib.dom types and a single asTemplateCtx view. Templates take HTMLElement / Document directly; the only runtime branch kept is the genuine off-DOM stage===null path. Removes every as-unknown-as HTMLElement|Document cast from templates and decks, and the structural BeatTimeline re-declaration in both reference decks (now reference a shared TemplateTimeline type). Move the deck-only templates operatorDossier / incidentPlate / haulCitations (and their CSS) into the local-calgary-v2 deck; they are no longer part of the shared L2 surface. Co-evolve the DOM-touching template tests onto happy-dom (per-file docblock; global vitest env unchanged) asserting against real rendering, including mount-marker presence and cleanup removal.
The policy / source-scan suites (tests/runtime/policy-*.test.ts, screenshot-determinism-source.test.ts, and their shared source-policy.ts AST framework) assert codebase structure, not runtime behavior. Their whole-tree AST scans starved under the behavior suite's parallel load and intermittently timed out at the 5s limit (PUL-Q003 / PUL-Q007), making pnpm test flaky. Move them to vitest.policy.config.ts, run via a new pnpm policy script (serial, 60s timeout), and exclude them from vitest.config.ts so pnpm test is behavior-only and flake-free. Wire pnpm policy as a blocking CI job and a blocking pre-commit hook so the identical violation set is still enforced. No policy check is dropped or weakened; the Biome complexity-gate override for the cluster and the src coverage include are untouched.
…ror; fuzz validator tests No src/ caller discriminated the AudioError subclasses (grep across src/ for `instanceof Audio*` is empty), so the nine-subclass hierarchy (AudioSoundError / AudioGroupError / AudioRangeError / AudioSourceError, etc.) collapses to one AudioError carrying a `category` discriminant (sound | group | source | range | option) plus terse per-category constructors. ctx.audio runtime behavior, the throwable surface, and every rejected input are unchanged; audio.test.ts asserts AudioError + category where it formerly asserted a subclass, with the same failure-mode coverage. Replace the brittle field-by-field `.each` loops for assertSceneModule, assertCompositionManifest, and assertAudioBedDeclaration with one representative assert per shape plus a shared seeded property fuzz (tests/runtime/validator-fuzz.ts) covering the omission / wrong-type / out-of-range / non-kebab-id malformed classes and asserting the offending field is named.
…category Final-QA found two regression oracles that did not actually bite: - validator-fuzz asserted the offending field is named via toContain, which a 2-char field like 'id' satisfies from 'invalid'/'identifier'; a generic no-field message passed. Now matches the field as a delimited token (mutation probe: a generic message fails 34 tests). - the AudioError 'option' category (thrown at 7 sites) had zero category assertions; every covering test used bare toThrow(AudioError), so a mis-categorization passed silently. Pinned via expectAudioError (mutation probe: option->range now fails the mute-arg test). Also drop the dead createPresenterController import left in scene-loader after the ctx extraction (construction lives in scene-loader-ctx).
Reframe helper/module comments to state each unit's design responsibility instead of narrating Sonar/Biome cognitive-complexity, nested-function, and related lint-gate appeasement.
Contributor
Author
CI status (workflow_dispatch run on this branch)8 of 9 jobs green, including the cross-engine WebKit E2E that can't run in the local dev sandbox:
SonarCloud — pre-existing baseline, not a regression in this change. The failure is the coverage gate, dominated by |
Cut history, review-cycle narration, and per-field prose JSDoc across the runtime, keeping the load-bearing invariants (activation/rng loader contributions, presenter isolation, chrome forced-visibility ordering, non-idempotent-unsubscribe guards) as one-line contracts. Comments only: no code, behavior, public API, or data-pulsar-* change. Net -2.6k comment lines; all exemption markers untouched.
…al-spreads
Apply behavior-identical readability cuts to the scene-loader and
navigation/composition cluster:
- Inline single-use micro-helpers hoisted only for the cognitive-complexity
gate: scene-loader setStageAttr/clearStageAttr, scene-loader-ctx
audioOutputPolicyFor/audioServiceOptions.
- Collapse the '...(x === undefined ? {} : { x })' idiom to '...(x ? { x } : {})'.
- Replace writable-intermediate-then-freeze construction in
parseNavigationSearch, composeSegments, and buildPrompterScript with
direct frozen literals using conditional spread.
- Merge the two-stage chromeBehavior extraction in scene-loader-guard.
Public signatures, data-pulsar-* attributes, error strings, and PUL-Q008
attribute-literal handling are unchanged; the complexity threshold stays
at 15 with no new suppressions.
Inline single-use timeline speed/repeat validators, rebuild
buildRunComposeOptions as one conditional-spread literal (drops four
as-cast assignments), and collapse the undefined-spread idiom to
...(x && { x }) for the sprite/mute fields in createHowlerAudioEngine.
Trim the audio-unlock-dom preamble, orphaned adapter JSDoc, and review
narration to terse contracts. Behavior, public signatures, error
strings, data-pulsar-* attributes, and the abort-race isolation guard
unchanged.
Make createDomWorkbenchChrome and createDomAudioUnlockAdapter generic over their concrete element type so main.ts mounts a real HTMLElement / HTMLButtonElement without `as unknown as Node` casts at the call site; the surviving ctx chrome cast is reduced to a single named L2->L1 boundary assertion. Trim narrative/ceremony comments in workbench-chrome, audio-unlock-dom, prompter-window, practice-renderer, and keyboard-source. Collapse the repeated tl.fromTo reveal boilerplate in the pulsar-intro deck behind a local reveal helper (byte-identical timeline output) and refresh the two decks' holdForever / fadeIn comments. Behavior, exported signatures, data-pulsar-* attributes, and tests are unchanged. The chrome applyMode mode guard (exported boundary, dedicated test) is preserved.
Extract the jsdom-free fakes that 10 test files re-derived: the mode-fixture stage stub (byte-identical across five tests/scenes fixture suites), the synthetic HTMLElement/Document tree shared by the chrome-slots and chrome-extras suites, and the event-emitting / no-op presenter controllers used by helpers and presenter-driven. Typing the chrome fake as the real DOM interfaces removes every as-unknown-as cast at those call sites; copy-pasted coverage-narration headers trimmed to one line. Assertions, per-file test counts, and behavior coverage unchanged.
Contributor
Author
Simplicity pass (5 follow-up commits)A dedicated verbosity sweep, driven by a 5-agent hunt that found ~1,186 LOC of code "no one would quite write that way." Applied in gate-verified batches (each: implement → adversarial review → typecheck + lint + test + policy green):
Net src 22.4k → 19.5k (−2,937, ~13%). The complexity gate ( |
… of coverage gate Adds real mount-and-assert behavioral suites for the under-tested reusable templates (terminal 35%->99%, card-carousel, activity-feed-payoff, chat-pick-list, split-dialogue-email, split-pane-terminal-doc, metric-ticker), the _shared envelope, the register barrel, and the abortable-timing primitives. Each asserts rendered DOM / beats / cleanup (mutation-probed: breaking a template's class fails the tests). The example decks are demonstration content exercised by the Playwright specs, not the product surface, so src/decks/** is excluded from the coverage gate (sonar + vitest) — the gate now measures the runtime + template library. Product line coverage ~93% -> ~99%.
The decks are demonstration content whose scenes share an intentionally repetitive build/beats scaffold; that is not product duplication. Same rationale as the coverage exclusion. New-code duplication was 9.1% (threshold 3%) driven ~98% by src/decks (1122 of ~1142 lines); the product surface (runtime + template library) carries ~20 duplicated lines, well under threshold.
Contributor
Author
SonarCloud — diagnosed and greenThe earlier SonarCloud failure was misattributed (by me) to deck coverage. Pulling the actual gate conditions against the healthy Fix: exclude the example decks from duplication detection ( Independently, the under-tested product surface got real behavioral tests: Verified green: all 9 CI jobs pass — including SonarCloud and the WebKit cross-engine E2E — confirmed via a PR-scoped analysis against the |
Architecture preflight for issue #162. Records the decision to replace the GSAP master-timeline sequencer with one imperative control plane (run scene -> await advance -> tear down -> next), a runtime-owned disposal registry, and GSAP demoted to a per-scene tool. Supersedes the master-timeline portions of ADR-003/011/015/016-021/023/024/025/026. Adds the issue-162 preflight design note and updates the ADR/design indices.
Issue #162 spike. Adds the activation-scoped control plane that replaces the GSAP master-timeline sequencer: a scene is an async body; ctx.sleep / ctx.hold / ctx.runTimeline are end-bound (reject SceneCancelled on advance or supersession) so the body unwinds with no abort-polling, and the runtime owns deterministic teardown via a disposal registry. runScenes sequences scenes (run -> await end -> tear down -> next). Self-contained happy-dom test proves sequencing, runtime-owned chrome reset between scenes, disposer + spawned-async teardown (no resurrection), runTimeline completion, and navigation supersession. The cold-open / placard ports + browser entry that prove L2 reuse against the gitignored local-calgary-v2 deck are kept local (not committed); the spike confirmed L2 needs zero edits to run on the new control plane.
…l plane Adds runSceneModules: sequences real SceneModules (create -> play the per-scene GSAP timeline standalone -> hold for advance -> cleanup) with no master timeline. The committed decks and every buildTemplateScene ride on this unchanged. Extracts shared createEndGate / holdUntilEnd / playUntilEnd helpers (dedup with the async-body runScene path). Test drives the real create/timeline/cleanup ordering across scenes.
Threads an optional preload hook (awaited before each scene mounts) and the scene's slice index (disambiguates repeated scene ids when building per-occurrence ctx) into the SceneModule sequencer, so the lean loader can reuse the asset preloader and per-occurrence activation/rng.
main.ts now boots present/prompter through createPresentLoader, which runs the resolved scene slice via runSceneModules — no master-timeline adapter, transition overlay, or scrub transport. _shared template envelope drops the advance-gate + deactivate-on-complete so the root stays visible until the run-loop holds for advance and cleanup removes it. SceneActivation moved to scene.ts. Browser-verified: pulsar-intro navigates pi-title -> pi-thesis via ArrowRight, one scene mounted at a time. NOTE: dead master-timeline modules + their tests are removed in the follow-up cleanup commit; suite is mid-migration here.
…es (ADR-032) Removes the dead L1 now that the live app runs on the imperative run-loop: - composition-resolver, scene-loader, presenter-transport, the master-timeline machinery in timeline.ts (composeMasterTimeline / MasterTimeline / advance gates), chrome/scrub, and system/transitions. - speculative workbench modes (standalone/loop/paused/scrub/screenshot/ rehearsal): trimmed navigation grammar + mode-profile to present/prompter, deleted the mode fixtures and their unit + e2e tests. - WorkbenchSceneCtx/WorkbenchChromeSlots moved to runtime/scene-ctx.ts. - onDeactivate template teardown moved into each scene's cleanup (run-loop calls it on scene exit, not at the timeline's natural end). - flickerOutAll returns a stop handle / accepts an AbortSignal so its staggered timers cannot fire after teardown. Tests asserting the deleted layer are removed; surviving-behavior tests rewritten for the two-mode reality. Net -20k LOC. Gates: typecheck + lint clean, 1106 unit tests + 1017 policy tests pass. Browser-verified: pulsar-intro navigates pi-title -> pi-thesis -> pi-scene through the run-loop with zero console errors.
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.
Summary
Re-architects the scene/composition runtime for maintainability, reliability, and authoring ergonomics, while preserving every binding contract. The work was driven by a diagnosis that the runtime — though correct — concentrated complexity in a handful of god-functions, ran DRAFT capabilities on the hot path, carried a test suite that asserted code structure more than runtime behavior, and imposed an authoring tax (mandatory factory wrappers +
as unknown ascasts) that made it harder to use than the hand-written reference decks.Stacked on
aces-pulsar-decks(its actual branch point); retarget tomainonce that base lands. 15 commits, behavior-preserving throughout — the existing test suite and thetests-e2e/Playwright specs were the regression oracle at every step.What changed
defineScene({ id, title, create, timeline, cleanup })is the single source of scene defaulting + validation; the other metadata fields default.buildTemplateScenedelegates to it.ModeProfiletable (mode-profile.ts) replaces ~6 scatteredmode === Xdecision sites (audio policy, slice truncation, chrome visibility, bed suppression, scrub cue gate, runner hints). Snapshot-equivalence tested across all 8 modes.scene-loaderdecomposed — thebuildLoad/runTargetgod-functions split into cohesive units (scene-loader-ctx.ts,scene-loader-guard.ts); the beat/mode grammar re-check now shares oneNAVIGATION_GRAMMARsource with the parser (defense-in-depth retained).createSceneLoader's public surface is byte-identical.presenter-transport.ts, wired only undermode=present(a non-present navigation never instantiates it; covered by a test).AbortSignaldisposal gate (no disposed-flag threading), unified bed registration throughload(), table-driven play-option validation, and the 9AudioErrorsubclasses collapsed to one categorizedAudioError.as unknown as HTMLElementcasts are gone (zero remain insrc/system/templatesandsrc/decks); DOM-touching tests opt intohappy-domper-file (global env staysnode); dead system-level templates removed.SceneActivationembedded inPlanStep, three finalizers collapsed to one, scene-failure isolation extracted into awithFailureIsolationdecorator over a bare testable engine. Both-path regression tests added (abort-mid-mount, create/timeline/cleanup-throw, repeated-id-one-fails).pnpm testinto a dedicated blockingpnpm policygate (CI job + pre-commit hook); enforcement proven preserved by injection (eval/Function/remote-import/banned-PRNG/gsap-in-scene all still flagged). Brittle field-by-field validator.eachloops replaced by a seeded property fuzz.Measured outcomes
asset-preloader.tsremains).runLifecycle,buildLoad,runTarget, and audioplay/unlock/normalizeSourcesnow pass the gate atmaxAllowedComplexity 15with no exemption.as unknown ascasts in templates/decks; realHTMLElement/Documenttypes; the defensive ctx-narrowing theater (isTemplateCtx/isStageShape/isGsapShape) removed.Honest notes
terminal); decks keep their thin localbuildScenehelpers (legitimate per-deck DRY). Verbosity was cut where it was accidental (god-functions, casts, fake-DOM types, DRAFT-on-hot-path, dead templates, brittle.eachtests, stale linter-appeasement comments) and kept where it is essential (contract validation, the blocking policy gate, useful contract docs).Verification
pnpm typecheck,pnpm lint(incl. complexity gate),pnpm test(1588, flake-free across repeated runs),pnpm policy(1035),pnpm build— all green.AudioErrorcategory) were fixed and mutation-proven before this PR.Preserved contracts
data-pulsar-*stage attributes (read by the E2E specs); URL grammar determinism; per-occurrence activation identity (#99); cleanup-exactly-once on every path; screenshot determinism; and the error-isolation behaviors prior reviews added for real bugs (presenter command-source throw envelope, chrome forced-visibility-before-cleanup, prompter caption isolation, non-idempotent unsubscribe).