Sub C: DSP test harness — Tier1→3b infrastructure (closes #4) - #12
Merged
Conversation
Implements Sub C of the Foundation epic (#1): - Python scipy golden generator under `scripts/gen-golden/` (uv-pinned), producing 8 canonical signals × passthrough preset to `packages/test-utils/golden/passthrough/`. - `@denaudio/test-utils` rebuilt with subpath exports: `./signals` (browser-safe — chirp/sine/impulse/dc/silence + null helpers) and `./node` (adds Node-only `wav.ts` RIFF reader + Tier2 `runner.ts`). Multi-entry Vite lib build emits each module separately so Playwright's in-page bundle never drags `node:fs` into Chromium. - Tier2 runner iterates over the EXPECTED (preset × signal) matrix and surfaces missing golden files as `missing golden: ...`, satisfying issue §7.2's "delete one file → fail" check. - `@denaudio/examples` Vite SPA scaffolding with hash-router + Canvas2D waveform/spectrogram + A/B player. Catalog probes the pipeline on an OfflineAudioContext so the page loads cleanly in headless Chromium; the realtime A/B player is gesture-gated. - Tier3a Playwright runs 3 OAC tests (chirp, sine_1k, impulse) at -96 dBFS, bridged through `window.__denTier3a` because page.evaluate cannot resolve bare-specifier imports. - Vercel native GH integration via repo-root `vercel.ts` + `scripts/vercel-install.sh` (rustup + binaryen-from-source) + `scripts/vercel-build.sh` (vp run build). No `.github/workflows/preview.yml`. - CI extended with `test-tier2` and `test-tier3a` jobs (both `needs: [wasm-build]`). Issue #4 body was updated 5 times during implementation as living-doc SSoT (per AGENTS.md §0.1) for: Vite version (^5.4 → ^6.0 to match the rest of the monorepo), `__dirname` → `import.meta.dirname`, subpath exports for test-utils, `--experimental-strip-types` for `node --test` on `.ts`, scipy chirp returns cos (not sin — caused diff RMS to blow past -3 dBFS), exposing the page→test bridge on `window`, and the `vp run --filter <pkg> <script>` argument ordering quirk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40aa922785
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The first version of these scripts had three real bugs that would have broken the very first Vercel deploy: 1. `vp` is installed to `$HOME/.vite-plus/bin/vp` (verified by reading the install script at https://vite.plus 2026-04-14), NOT `$HOME/.local/bin/vp` as the original PATH export assumed. After `curl ... | bash` returned, the `vp install --frozen-lockfile` on the next line would have failed with `command not found`. 2. `command -v vp` (and `command -v cargo`) was used as the install guard, but Vercel runs the script in a fresh non-interactive bash that doesn't source `~/.bashrc`. On warm builds where the binary exists but isn't yet on PATH, the guard would falsely report "not installed" and re-run the installer needlessly. Switched to direct binary file checks (`[ -x "$HOME/.cargo/bin/cargo" ]`). 3. The Vite+ installer prompts interactively about whether to enable its bundled Node.js manager when no $VP_NODE_MANAGER env is set, which would have hung Vercel's CI shell forever. Now sets `VP_NODE_MANAGER=yes` explicitly so vp's bundled node + npm + npx shims (which read `.node-version`) are installed unconditionally. Also: `.` (source) of `~/.cargo/env` is now guarded with `[ -f ... ]` so a system-installed cargo doesn't trip `set -e` when rustup hasn't written that file. End-of-install summary echoes resolved binary paths so first-run logs make the toolchain layout obvious. vercel-build.sh's PATH now includes all three install dirs (~/.vite-plus/bin, ~/.local/bin, ~/.cargo/bin) explicitly, mirroring vercel-install.sh — necessary because Vercel runs install + build in separate shell invocations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Two real issues codex flagged on the original commit: P2 (vercel-install.sh): the rustup install branch installed the wasm32-unknown-unknown target, but the install was guarded by `! command -v cargo`. On warm Vercel builds where ~/.cargo/bin is cached but ~/.rustup/toolchains/.../wasm32-unknown-unknown/ is not, or in environments with a system-preinstalled cargo (no rustup), subsequent `cargo build --target wasm32-unknown-unknown` would fail with a missing-target error. Now `rustup target add wasm32-unknown-unknown` runs unconditionally outside the bootstrap branch (idempotent — no-op when the target is already present). P2 (passthrough.spec.ts): the Tier3a null test only returned `getChannelData(0)` from the rendered offline buffer and compared it to `golden.samples[0]` (left only). Tier2's runner already compares both channels via `rmsDiffDbFs(actual, golden.samples, ...)`. The asymmetry meant a future worklet bug that affected only the right channel (stereo widener, ping-pong delay, etc.) would slip past CI's stereo-verification check. Now both channels are returned and compared. P1 from the same review (unconditional `~/.cargo/env` source) was already addressed in commit b129372 — codex reviewed the original 40aa922 before that fix landed. Issue #4 §6.5 body updated to reflect the both-channel snippet. Also gitignored `.claude/` session-state dir that Claude Code creates locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review Re-review request after addressing the previous round (commit
Please verify the fixes and check anything else you find. |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Two visible bugs the catalog page screenshot exposed: 1. mountWaveform drew both L and R into the SAME vertical band ([0, h], with `(1-min)*h/2` y-coords). Whichever channel was drawn second (R, in red) completely overwrote the first (L, in blue). For Passthrough where L == R the L stroke was 100% hidden. Now L takes the top half and R takes the bottom (DAW-style stacked layout), with a faint center line per band for amplitude reference. Single-channel buffers fall back to the full canvas. 2. mountSpectrogram called `g.putImageData(bmp, 0, 0)` to write a small `frames × (SPEC_FFT/2)` bitmap to the top-left of the display canvas, then `g.drawImage(canvas, ...)` to scale it up — reading from the same canvas it was writing to. Per the Canvas2D spec, src/dest overlap is undefined, and in practice the result was just the tiny bitmap stuck in the corner. Now renders to a separate offscreen canvas and drawImage-scales onto the display canvas. Also fixed the "log-freq" claim: the old code used a linear bin → row mapping (`row = SPEC_FFT/2 - 1 - k`), so a log chirp would show up as a near-vertical streak on the right. New version precomputes a log-freq display-row → FFT-bin lookup (top = Nyquist, bottom = 20 Hz) so a log chirp draws as the expected diagonal. FFT magnitudes are now normalized by `SPEC_FFT/2` so a full-scale sinusoid lands near 0 dBFS instead of saturating at +50 dBFS, making the (db+96)/96 → [0,1] colormap behave intuitively. No functional change to the WASM/worklet/test paths; widgets only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…+ clearer labels UX feedback from the deployed catalog page: 1. Switching the Signal selector left the wave/spectrogram canvases stale until you hit "Re-render wave + spec". mountABPlayer now accepts an optional `onSignalChange` callback that the page wires to refreshViz, so the visualizers update automatically. If the realtime player is already playing, the source signal is also restarted so the audio matches. 2. The Wet/Dry slider and the visualizer were operating on different pipelines (slider = realtime mix, viz = always wet via offline render through the effect), but neither the labels nor the surrounding copy made that clear. Added a caption between the player and the visualizers explaining the split, and renamed the widget headers to "Waveform — effect output (L top / R bottom)" and "Spectrogram — effect output (L top / R bottom, log-freq …)". 3. mountSpectrogram only rendered channel 0. Now stacks L on the top half and R on the bottom (matching the waveform widget's layout) with a faint divider line between them. L gets the existing blue- tinted colormap, R gets a red-tinted one so the bands are distinguishable even when L == R (Passthrough's case). The log- freq bin lookup is computed once per band height and reused for both channels. No change to the worklet/test paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The "Re-render wave + spec" button became dead weight after the auto-refresh on signal change landed: - Signal change auto-refreshes via mountABPlayer's onSignalChange. - Wet/Dry only affects realtime playback; visualizers always show the wet effect output, so the slider can't change what's drawn. - Sub D's Gain (and any future effect) will wire its param sliders to call refreshViz directly, so no manual button needed there either. - Window resize is the one remaining case the button could help with, but the canvases are pinned to mount-time clientWidth × DPR and won't redraw correctly even with a manual click; proper resize support would need ResizeObserver and is out of scope here. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
24 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.
Summary
Implements Sub C of the Foundation epic — the DSP verification infrastructure that Sub D (Gain) and every future effect will slot into without inventing testing conventions. Validates end-to-end on the Sub B Passthrough stub.
Deliverables (all from issue #4):
scripts/gen-golden/— Python + scipy + uv. Produces 8 canonical signals × passthrough preset →packages/test-utils/golden/passthrough/. Committed to repo (issue D6).@denaudio/test-utils— subpath exports (./signalsbrowser-safe,./nodeaddsnode:fs-using wav reader + Tier2 runner). Multi-entry Vite lib build.(preset × signal)matrix; surfaces a missing golden file asmissing golden: ...so issue §7.2's "delete one file → fail" check works.@denaudio/examples— Vite SPA with hash router + Canvas2D waveform/spectrogram + A/B player. Pipeline probe runs onOfflineAudioContext(no user gesture) so the page loads in headless Chromium; realtime A/B player is gesture-gated behind an Enable button.window.__denTier3abecausepage.evaluatecannot resolve bare-specifier imports.vercel.ts+scripts/vercel-install.sh(rustup + binaryen-from-source) +scripts/vercel-build.sh(vp run build). No.github/workflows/preview.yml.test-tier2andtest-tier3ajobs.Links
§0.6 Disclosure — issue body updates during implementation
Per AGENTS.md §0.1 the issue body is treated as living-doc SSoT. While implementing Sub C I found gaps and applied 5 rounds of body edits (no comment-style addenda) before writing the corresponding code:
vite ^5.4.0for examples did NOT match Sub B's actual^6.0.0everywhere else, and the prose claim "matches packages/core" was factually wrongvite ^6.0.0to keep one Vite major across the monorepo; rewrote the prose.__dirnameused inrunner.ts/ Tier2 test / Playwright test, but those files live in"type": "module"packagesimport.meta.dirname(Node 20.11+, non-experimental). Same pattern as Sub B'sbuild-wasm.mjs. Also added/// <reference types="node" />to runner/wav/test files becausevp check --type-aware(oxlint + tsgolint) doesn't auto-load@types/node'sImportMetaaugmentation under"moduleResolution": "Bundler".test-utils/src/index.tsre-exportingwav.ts(which usesnode:fs) into a single bundle thatpage.evaluatewould importtest-utilsnow has subpath exports./signalsand./node. Browser-side imports/signals; Node-side imports/node. No default.entry.node --test packages/test-utils/testsdid not work in Node 22 < 22.18 (required--experimental-strip-types) AND failed for directory paths under strip-typesnode --experimental-strip-types --test 'packages/test-utils/tests/**/*.test.ts'(glob).> **Parent**: #<parent-issue-number>placeholder> **Parent**: #1cos(phase + phi), but the TS port usedMath.sin(phase). First sample diff was 0 vs 0.7079, blowing diff RMS to -3 dBFSchirpLogto useMath.cos. Now diff vs Python golden is < -190 dBFS for chirp.await import("@denaudio/effects")insidepage.evaluate— bare specifiers don't resolve in browser without a bundlerwindow.__denTier3a = { Passthrough, CANONICAL, workletUrl }; spec reads from window.vp run <script> --filter <pkg>— but Vite+ parses--filteras a script arg unless it comes BEFORE the script name. Hit "Task X not found"vp run --filter <pkg> <script>(or-F) everywhere.processor.jscontent as adata:video/mp2tURL when Vite builds it (the runtimeaudioWorklet.addModulethen chokes on raw TS source). Issue #3 §8 Fallback #2 already noted this and deferred Playwright smoke to Sub CworkletUrlexplicitly via Vite?urlimport on the workspace-relative dist path. Properly fixing Sub B's URL emission is out of scope for Sub C; flagged for a follow-up.All 9 changes are now reflected in the issue #4 body — please re-read the issue before reviewing the diff.
Maintainer setup reminder
Vercel native GH integration is one-time manual setup at https://vercel.com (Add Project → import
yuichkun/den→ set Install/Build commands per CONTRIBUTING.md "Maintainer setup"). The PR cannot configure this from the runner — please do this once before merge so Tier3b becomes "green = preview URL posted". Issue §8 Fallback #1 covers the not-yet-set-up case.Definition of done
scripts/gen-golden/exists withpyproject.toml,signals.py,gen.py,effects.py,README.mdvp run gen-goldenproduces 8 passthrough goldens (chirp/pink/sine_1k/5k/10k/impulse/dc_half/silence)@denaudio/test-utilssubpath exports./signals+./node, no default.vp run test:tier2green at -96 dBFS (7 signals — pink omitted from TS CANONICAL by design)vp run --filter @denaudio/examples devserves/#/passthroughpackages/examples/tests/passthrough.spec.tsruns 3 chromium tests, all green locallytest-tier2+test-tier3ajobs, nopreview.ymlvercel.tsmirrors dashboard config;scripts/vercel-*.shmode100755mv golden/.../default__chirp.wav /tmp && vp run test:tier2exits withmissing golden: ...cargo fmt --check,clippy --workspace --exclude den-core,clippy -p den-core --target wasm32-unknown-unknownall green (Tier1 untouched)vp check+vp run -r typecheckgreenvp run gen-golden/vp run test:tier2/3a/testTest plan
vp install && vp run build && vp run gen-golden && vp run testfrom a fresh clonevp run --filter @denaudio/examples dev→ http://localhost:5173/#/passthrough — pipeline-ready probe showspipeline ready ✓, click "Enable A/B player", select chirp, hit Play, confirm waveform/spectrogram render🤖 Generated with Claude Code