Migrate Sound Module to Conductor - #796
Merged
Merged
Conversation
Splits midi into a pure, evaluator-free functions.ts (unchanged signatures) and a Conductor-facing index.ts plugin, since sound and stereo_sound import midi_note_to_frequency and friends directly as plain TypeScript and sound/stereo_sound's own Source-facing APIs re-export several of these functions. Migrating index.ts's exports to require an IDataHandler would have broken both call sites immediately. - functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic - conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor list conversion, accidental validation) used by the plugin; kept separate from index.ts so they stay importable from vitest, which hits a decorator syntax error importing index.ts directly - index.ts: BaseModulePlugin subclass wrapping the pure functions; SHARP/FLAT/NATURAL are pushed onto `exports` directly in the constructor since BaseModulePlugin.initialise() only registers exportedNames that are functions - Includes the same __bindExportedMethods() workaround as repeat/rune/binary_tree for the unbound-method issue in BaseModulePlugin.initialise() (source-academy/conductor#41) - sound/stereo_sound's functions.ts and index.ts updated to import midi's pure functions from the new `/functions` subpath instead of the bundle root, which now exports the Conductor plugin
Function.prototype.name gets mangled under minification, which would turn these error messages into cryptic garbage like "t expects...". Two of these were carried over unchanged from the original module; the third is in the new Conductor-facing index.ts.
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
…back scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is unavailable under Conductor's module loader (the require() shim it's given is a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for any scale function. Build the intermediate list with a plain array tuple instead, since Pair<H, T> is just [H, T] structurally. Also addresses Aarav's review comments on #791: - Removes __bindExportedMethods now that the upstream binding fix (source-academy/conductor#41) is merged and published. - Widens letter_name_to_midi_note/midi_note_to_letter_name/ letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/ get_accidental/key_signature_to_key's note/accidental parameters to plain string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in index.ts. Doing this exposed two real validation gaps that the casts had been silently papering over: add_octave_to_note never validated `note` at all (just interpolated it into the result string), and midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP accidental as FLAT instead of rejecting it. Both now throw EvaluatorParameterTypeError for invalid input, with regression tests added.
…cy resolution scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type shape, not js-slang itself. Replaced with a local Scale type; js-slang moves to devDependencies since only the test suite still uses it. Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub URL, which yarn.lock had resolved and locked to a commit from before even the BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using a local portal: link for testing) was building against that stale, broken conductor entirely silently. Switched to the same versioned npm range other bundles already use (^0.7.0), and reverted the two calls to assertNumberWithinRange that had been written against conductor PR #43's still-unpublished options-object signature back to the options actually published in 0.7.0.
…igrate-midi Resolved conflicts by keeping midi's already-Conductor-migrated code (class-based MidiModulePlugin, conductor/common errors, js-slang-free Scale type) over conductor-migration's stale pre-migration content, same pattern as feat/migrate-binary-tree. Also fixed an unrelated import-path conflict in sound/stereo_sound's functions.ts (combining both sides' imports) and a pre-existing type error in midi's test that surfaced once real type-checking ran again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Picked up as part of merging conductor-migration in - noImplicitOverride is now enabled repo-wide, and exportedNames/channelAttach shadow members declared on BaseModulePlugin. Also fixed a test that built its input scale via js-slang's untyped list() instead of midi's own js-slang-free Scale shape, which only surfaced as a real tsc error once the merge brought stricter checking back online. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Scraps the earlier plugins-repo sound-io plugin pair in favor of the pattern confirmed against rune's migration (PR #765): SoundModulePlugin declares its own channelAttach directly, and modules/src/tabs/Sound implements IPlugin/SoundTabRpc as the host-side counterpart, talking over Conductor's makeRpc helper - no separate runner/web plugin package needed at all. Also fixes plotly's draw_sound_2d, the one other consumer of bundle-sound's types, for the Wave type's redesign from a plain (t: number) => number to an async generator (so that stepping and nested user closures evaluate correctly on the CSE machine). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the midi and sound bundles to integrate with the @sourceacademy/conductor framework, enabling generator-based wave functions that preserve debugger stepping and breakpoints. Audio I/O operations are offloaded to a host-bridged RPC interface (SoundTabRpc) since browser audio APIs are unavailable in the sandboxed runner Worker, and the Sound tab is rewritten as a React-based plugin. Review feedback highlights critical performance issues in consecutively and simultaneously where using reduce to nest generator functions leads to severe performance degradation at 44100 Hz; refactoring these to use flat loops is recommended. Additionally, a guard should be added to linear_decay to prevent NaN propagation when the decay period is zero.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Retires stereo_sound as a separate bundle/tab entirely. Sound becomes
{leftWave, rightWave, duration} always; "mono" isn't a separate type,
it's just the common case where leftWave === rightWave (same
reference), produced by make_sound. make_stereo_sound builds a
genuinely stereo Sound from two different waves. Every combinator
(consecutively/simultaneously/adsr/phase_mod/stacking_adsr/
instruments) now operates on this one shape via small per-channel
helpers (joinWaves/sumWaves/adsrWave/phaseModWave), so composing a
"mono" sound with a stereo one is a non-issue - there's nothing to
convert, and none of stereo_sound's oscillator/envelope math is
duplicated a second time the way it was as a separate bundle.
get_wave/get_left_wave/get_right_wave: get_wave keeps meaning "the"
wave (== left channel), so existing SICP-style code (play(sine_sound(...)),
get_wave(sound)) keeps working unchanged for the common mono case.
Adds a lower Wave-returning layer alongside the existing Sound layer
(sine_wave/square_wave/triangle_wave/sawtooth_wave/noise_wave/
silence_wave), with sine_sound etc. as thin convenience wrappers
(sine_sound = (freq, dur) => make_sound(sine_wave(freq), dur)).
record()/record_for() use however many channels the input device
actually has - a mono microphone (the common case) produces a Sound
whose left and right channels are the same wave; no separate
record_stereo. play/samplesToSound skip redoing work for a mono
Sound (sampled once, not twice) by checking leftWave === rightWave.
New stereo-specific operations: make_stereo_sound, play_waves,
pan, pan_mod, squash, get_left_wave, get_right_wave.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nification
Continuation of the previous commit (which only picked up the
stereo_sound/StereoSound deletions due to a failed multi-pathspec git
add) - this is the actual Sound={leftWave,rightWave,duration} rework
of the sound bundle and its tab described there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…igrate-sound Resolved conflicts by keeping the already-Conductor-migrated code (midi, sound, and stereo_sound's retirement) over conductor-migration's stale pre-migration content, same pattern as feat/migrate-binary-tree and feat/migrate-midi. Also re-applies the noImplicitOverride fix and the midi scale-test fix that landed separately on feat/migrate-midi (this branch has its own independent midi history). yarn.lock resolved per the review feedback on PR #795 (reset to origin/conductor-migration's copy first, then reinstalled, so only new/changed dependencies actually move instead of picking up whatever was stale on this branch); .yarnrc.yml also gets @sourceacademy/conductor added to npmPreapprovedPackages for the same reason as PR #795. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#795 (fix/conductor-dependency-pin) is about to merge into conductor-migration first, establishing @sourceacademy/conductor in the yarn catalog and npmPreapprovedPackages. Switching midi/sound/ tab-Sound's package.json over to "catalog:" now (matching #795 verbatim) avoids re-doing this exact reconciliation the next time this branch merges conductor-migration in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses Gemini's high-priority review comments on PR #796: reduce() was nesting joinWaves/sumWaves closures sounds.length deep, so sampling near the end of a long chain meant up to N nested async generator delegations per sample at 44100 Hz - real overhead for async generators that plain synchronous functions (the pre-migration implementation) never had. Rewritten as a flat scan that picks the active sound directly and yield*s into it once, keeping delegation depth O(1) regardless of how many sounds are combined, while still threading through user closures correctly. Also fixes a real (if narrow) NaN: linear_decay(0) computed 1 - 0/0 when release_ratio (or a future caller with decay_ratio) is exactly 0, corrupting the sample at that instant. Sample rate/instrument functions never happened to hit it because of how the surrounding branch conditions are structured, but adsr's release branch can. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Akshay-2007-1
marked this pull request as ready for review
July 16, 2026 05:24
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Matches the same fix already applied on #795 and #796: adds @sourceacademy/conductor to the yarn catalog and npmPreapprovedPackages, and switches midi's own package.json to "catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't leave midi on the old convention once #795 lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalkthroughThe PR migrates MIDI and sound bundles to Conductor plugins, changes sound processing to asynchronous stereo generators, adds browser audio RPC integration, removes legacy stereo-sound and audio-tab artifacts, and updates tests, package wiring, and Plotly sampling. ChangesConductor migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant SoundModulePlugin
participant SoundFunctions
participant SoundTabPlugin
participant BrowserAudio
Program->>SoundModulePlugin: call sound module method
SoundModulePlugin->>SoundFunctions: convert Conductor values
SoundFunctions->>SoundTabPlugin: request playback or recording
SoundTabPlugin->>BrowserAudio: play samples or capture microphone
BrowserAudio-->>SoundTabPlugin: completion or recorded PCM
SoundTabPlugin-->>SoundFunctions: return RPC result
SoundFunctions-->>SoundModulePlugin: return Conductor value
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundles/plotly/src/functions.ts (1)
264-286: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCap waveform sampling to a display-sized point count.
This performs 44,100 serial generator drains per second of audio. Generator/Promise overhead and oversized Plotly datasets can freeze the UI for ordinary multi-second sounds. Sample using a stride capped to a reasonable maximum point count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundles/plotly/src/functions.ts` around lines 264 - 286, Update draw_sound_2d so waveform sampling uses a stride that limits the generated time_stamps and channel arrays to a reasonable display-sized maximum point count instead of iterating once per 44,100 Hz sample. Apply the stride consistently to the sampled time, wave invocation, and loop bounds while preserving the existing generator-draining behavior and output rendering.
🧹 Nitpick comments (1)
src/bundles/midi/src/__tests__/functions.test.ts (1)
71-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression cases for invalid bare notes.
Cover
B#,E#,Cb, andFb, plus lowercase normalization, soadd_octave_to_notecannot return values rejected by the canonical parser.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundles/midi/src/__tests__/functions.test.ts` around lines 71 - 89, Add regression tests in the add_octave_to_note suite for invalid bare notes B#, E#, Cb, and Fb, asserting each throws the canonical validation error rather than producing an octave-appended value. Also add lowercase-input cases to verify supported lowercase notes are normalized correctly while invalid lowercase accidental forms remain rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundles/midi/src/functions.ts`:
- Around line 150-159: Update add_octave_to_note in
src/bundles/midi/src/functions.ts:150-159 to use parseNoteWithOctave as the
canonical validator, reject inputs that already contain octave digits, and
normalize lowercase note names before returning. Extend the add_octave_to_note
tests in src/bundles/midi/src/__tests__/functions.test.ts:71-89 with B#, E#, Cb,
Fb, and lowercase-normalization cases.
In `@src/bundles/sound/src/functions.ts`:
- Around line 586-596: Update consecutively and the corresponding simultaneous,
transformation, and phase-modulation functions in
src/bundles/sound/src/functions.ts at lines 586-596, 623-633, 677-694, and
736-742 so mono inputs reuse the same generated wave by reference for both
channels, while stereo behavior remains unchanged. Add identity assertions in
src/bundles/sound/src/__tests__/sound.test.ts at lines 185-280 for mono
composition and 331-339 for mono ADSR preservation.
- Around line 776-786: Update pan to evaluate the squashed source once per
timestamp and reuse that sample for both stereo channels instead of
independently sampling through gainWave. In the modulation path around the
related functions at src/bundles/sound/src/functions.ts:803-817, share both the
source sample and modulation amount between channels. Add stateful-wave and
invocation-count regression cases at
src/bundles/sound/src/__tests__/sound.test.ts:298-329.
- Around line 431-446: Update the playback state flow around the sound-start
function and stop() in src/bundles/sound/src/functions.ts lines 431-446 to track
a generation token, incrementing it when playback starts or stops and clearing
globalVars.isPlaying in the promise finally handler only when its token is still
current. Add coverage in src/bundles/sound/src/__tests__/sound.test.ts lines
88-183 for stop-A/start-B/settle-A ordering, verifying stale playback completion
cannot clear the newer playback state.
- Around line 283-319: Update record and the related recording API around the
returned stop callbacks to synchronously reserve recording state before
scheduling any signals, rejecting calls while a recording is pending or active.
Reset the state only after stopRecording() settles, including failure paths, and
make each stop callback idempotent so repeated invocations cannot trigger
duplicate stops or signals.
- Around line 94-100: Update validateDuration to reject non-finite numeric
durations in addition to non-number and negative values. Use a finite-number
check so NaN and both infinities throw the existing appropriate validation error
before playback or allocation proceeds.
In `@src/bundles/sound/src/index.ts`:
- Around line 4-5: Update the documentation for the Wave contract to state that
a wave accepts time t and returns an async generator whose return value is the
amplitude, replacing the description that it directly returns a number.
- Around line 492-507: Validate each value extracted by the envelope-list
traversal before storing or invoking it as a closure. Update the logic around
envelopeClosures and closure_call_unchecked so non-closure elements are rejected
through the module’s established type-validation/error path, while valid closure
elements retain the existing harmonic processing behavior.
- Around line 95-102: Update closureToWave to validate the result returned by
evaluator.closure_call_unchecked before using result.value as the wave sample:
require the returned DataType to be NUMBER and ensure the runtime value is a
valid number, rejecting or otherwise handling non-number and NaN results
according to the surrounding wave error conventions. Do not rely on the existing
TypeScript cast as runtime validation.
- Around line 136-145: Update the Sound unwrapping return logic around leftWave
and rightWave so that when leftTv and rightTv contain the same closure value,
both channels reuse a single closureToWave result and preserve leftWave ===
rightWave; continue creating separate wrappers for distinct closures.
In `@src/tabs/Sound/src/index.tsx`:
- Around line 100-108: Update requestMicPermission() to stop all tracks on the
existing __mediaStream and clear it before calling
navigator.mediaDevices.getUserMedia. Then reacquire the stream and update
__micGranted as currently handled, ensuring a denied request cannot leave a
reusable previous stream.
- Around line 140-155: Update startRecording in src/tabs/Sound/src/index.tsx:
await the MediaRecorder start event before resolving and reject the promise when
the recorder emits error, while preserving the existing setup and status
transition. Update both affected mock/test sites in
src/tabs/Sound/src/__tests__/index.test.ts (lines 74-82 and 159-163) so start
fires asynchronously and the tests verify startRecording remains pending until
that event.
---
Outside diff comments:
In `@src/bundles/plotly/src/functions.ts`:
- Around line 264-286: Update draw_sound_2d so waveform sampling uses a stride
that limits the generated time_stamps and channel arrays to a reasonable
display-sized maximum point count instead of iterating once per 44,100 Hz
sample. Apply the stride consistently to the sampled time, wave invocation, and
loop bounds while preserving the existing generator-draining behavior and output
rendering.
---
Nitpick comments:
In `@src/bundles/midi/src/__tests__/functions.test.ts`:
- Around line 71-89: Add regression tests in the add_octave_to_note suite for
invalid bare notes B#, E#, Cb, and Fb, asserting each throws the canonical
validation error rather than producing an octave-appended value. Also add
lowercase-input cases to verify supported lowercase notes are normalized
correctly while invalid lowercase accidental forms remain rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1116f4c-268c-4ba8-902d-306f053f5896
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.yarnrc.ymlsrc/bundles/midi/package.jsonsrc/bundles/midi/src/__tests__/functions.test.tssrc/bundles/midi/src/__tests__/index.test.tssrc/bundles/midi/src/conductorAdapters.tssrc/bundles/midi/src/functions.tssrc/bundles/midi/src/index.tssrc/bundles/midi/src/scales.tssrc/bundles/midi/src/utils.tssrc/bundles/midi/tsconfig.jsonsrc/bundles/plotly/src/functions.tssrc/bundles/sound/package.jsonsrc/bundles/sound/src/__tests__/recording.test.tssrc/bundles/sound/src/__tests__/sound.test.tssrc/bundles/sound/src/__tests__/utils.tssrc/bundles/sound/src/functions.tssrc/bundles/sound/src/index.tssrc/bundles/sound/src/play_in_tab.tssrc/bundles/sound/src/protocol.tssrc/bundles/sound/src/riffwave.tssrc/bundles/sound/src/types.tssrc/bundles/sound/tsconfig.jsonsrc/bundles/stereo_sound/manifest.jsonsrc/bundles/stereo_sound/package.jsonsrc/bundles/stereo_sound/src/__tests__/index.test.tssrc/bundles/stereo_sound/src/functions.tssrc/bundles/stereo_sound/src/index.tssrc/bundles/stereo_sound/src/riffwave.tssrc/bundles/stereo_sound/src/types.tssrc/bundles/stereo_sound/tsconfig.jsonsrc/tabs/Sound/index.tsxsrc/tabs/Sound/package.jsonsrc/tabs/Sound/src/__tests__/index.test.tssrc/tabs/Sound/src/index.tsxsrc/tabs/Sound/tsconfig.jsonsrc/tabs/StereoSound/package.jsonsrc/tabs/StereoSound/src/__tests__/index.test.tsxsrc/tabs/StereoSound/src/index.tsxsrc/tabs/StereoSound/tsconfig.json
💤 Files with no reviewable changes (15)
- src/bundles/stereo_sound/tsconfig.json
- src/tabs/StereoSound/src/index.tsx
- src/tabs/StereoSound/tsconfig.json
- src/bundles/stereo_sound/src/tests/index.test.ts
- src/bundles/sound/src/play_in_tab.ts
- src/bundles/stereo_sound/package.json
- src/bundles/stereo_sound/src/riffwave.ts
- src/tabs/StereoSound/package.json
- src/bundles/sound/src/riffwave.ts
- src/bundles/stereo_sound/src/types.ts
- src/bundles/stereo_sound/manifest.json
- src/tabs/Sound/index.tsx
- src/bundles/stereo_sound/src/index.ts
- src/tabs/StereoSound/src/tests/index.test.tsx
- src/bundles/stereo_sound/src/functions.ts
- validateDuration now rejects NaN/Infinity, not just negatives (both used to slip through and crash sampleWave's Float32Array allocation with a raw RangeError instead of a clean module error). - closureToWave validates the closure's result is actually a number before returning it - a student-supplied wave returning e.g. a string previously corrupted playback silently via `as number`. - stacking_adsr validates each envelope-list element is a closure before invoking it, for the same reason. - consecutively/simultaneously/adsr/phase_mod/conductorToSound all preserve the "mono means leftWave === rightWave" invariant again - building both channels independently (even from identical inputs) produced two behaviourally-identical-but-distinct waves, silently doubling sampling work and losing the fast path in play()/etc. - play()/stop() guard against a stale playSamples() completion clobbering a newer play()'s isPlaying state via a generation token (stop-A/start-B/late-settle-A ordering). - tab: requestMicPermission() disposes the previous MediaStream before re-requesting, so a denied re-request can't leave stale tracks running and reusable by startRecording(). - tab: startRecording() now actually awaits MediaRecorder's start event (and rejects on error) instead of resolving as soon as .start() returns, matching the SoundTabRpc contract. Not addressed (flagged as false positive or out of scope for a quick fix, not silently dropped): - The module doc's "a wave returns a number" description is correct as written - it's the student-facing Source-language contract, not the internal async-generator implementation detail (which is already documented separately on the Wave type in types.ts). - Overlapping/concurrent recording sessions (record/record_for only gate on isPlaying, not a dedicated recording-state flag) and pan/pan_mod sampling the shared source/modulator wave twice per channel are both real but non-trivial fixes requiring more substantial state-tracking/sampling-order changes; left for a follow-up rather than a rushed partial fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Akshay-2007-1
changed the base branch from
conductor-migration
to
feat/migrate-midi
July 16, 2026 06:16
…rate-sound Needed after retargeting PR #796's base to feat/migrate-midi (stacking it on top of midi's PR instead of conductor-migration): resolved by keeping sound's already-migrated+CodeRabbit-fixed functions.ts (midi's branch still had the stale pre-migration copy) and re-deleting stereo_sound (midi's branch never touched it, so git saw sound's deletion as conflicting with midi's own unrelated merge-driven edits to it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e module doc Partial concession on CodeRabbit's doc-comment finding: the primary description stays as the Source-facing "number -> number" contract (that's the actual, correct student experience - the CSE machine threads the async-generator machinery transparently), but adds a one-line pointer to where the internal TS contract is documented, for maintainers reading this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ave_to_note Addresses CodeRabbit's review comment on PR #796 (surfaced there via shared branch history with sound, but the finding is midi's own): add_octave_to_note's inline regex accepted note spellings that noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb - accidentals that don't exist for those note names), and let lowercase input escape unnormalized through a type assertion. Now validates via parseNoteWithOctave (rejecting digits upfront, since that function alone would accept an octave already being present) and reconstructs using the normalized note name, preserving the original accidental spelling exactly as given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…igrate-sound Take conductor-migration's midi (already reviewed/merged as #791) over this branch's stale pre-review copy of the same migration. Keep our side for sound/functions.ts and sound/index.ts headers, since conductor-migration's conflicting version there just predates this PR's actual sound migration. Keep stereo_sound deleted per fa82ab1 (sound absorbed it: Sound is always stereo now), discarding conductor-migration's import-path-only edit to the now-retired bundle.
This was
linked to
issues
Jul 23, 2026
leeyi45
reviewed
Jul 23, 2026
leeyi45
left a comment
leeyi45
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the hard work, left some comments.
If you think that there are design decisions that should be documented (either for clarity or to avoid people "fixing" something they shouldn't) you should add docs. The docserver has a section specifically for bundle/module specific dev docs.
- record/record_for: stop hardcoding the function name in error messages
(record: ..., record_for: ...) - use `${record.name}`/`${record_for.name}`
like every other error in this file, so a rename can't silently go stale.
Same fix for play's EvaluatorParameterTypeError/duration-negative error,
which had the same hardcoded-literal bug.
- record/record_for: replace the nested setTimeout+.then() towers with a
single async IIFE using es-toolkit's delay(), preserving the exact same
event ordering (pre-recording-signal pause, recording signal, pre-recording
pause, recording, recording signal) but as flat sequential awaits.
Contributor
Author
|
Thanks - addressed the four inline comments (replies on each thread). For the "design decisions that should be documented" point: added a new page under the existing conductor-interop docs (#814) covering why |
py-slang's module interface no longer has a distinct "pair" representation (source-academy/py-slang#307): pythonToModule now builds every Python list/ pair as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A Sound built here via soundToConductor and round-tripped out to Python (assigned to a variable, passed to another module call like get_wave/is_sound/play) then arrives tagged ARRAY, not PAIR - conductorToSound/is_sound hardcoded `value.type !== DataType.PAIR` checks broke on this, same class of bug as binary_tree's #813. Fixed the same way: isPairLike(value) accepts either DataType.PAIR or DataType.ARRAY wherever a Sound's own tag is checked - pair_head/pair_tail already read either shape identically (py-slang's GenericDataHandler bridge), so this is purely about validation, not traversal. Also fixed conductorListToSounds (consecutively/simultaneously's list argument) and stacking_adsr's envelopes list: a genuine Python list of any length now crosses as a flat ARRAY too, not just a 2-element pair, so the old PAIR-chain-only walk would silently return zero elements for a real multi-sound list. Added readListElements to read either an ARRAY (via array_length/array_get) or a PAIR/EMPTY_LIST chain, matching py-slang's own GenericDataHandler.readListElements pattern. soundToConductor's own construction (pair_make calls) is unchanged - it always freshly builds a genuine PAIR; only validation of an incoming (possibly round-tripped) value needed to widen.
… no .sync twin
closure_call_sync lives on GenericDataHandler, the shared IDataHandler
implementation across all of py-slang's engines - it's always present
regardless of which engine is actually running, so closureToWave's old check
("does the method exist") was never actually engine-specific despite its own
comment claiming otherwise. Only some py2js closures ever carry a real
.sync twin; CSE and PVML closures never do. Every student-authored wave
function played via play()/play_wave() on CSE or PVML was hitting the
'Internal error: closure_call_sync unexpectedly had no synchronous form'
throw on its very first sample, since .sync was attached unconditionally
whenever the method merely existed.
Fixed by determining sync-capability once, with a real probe call, before
ever exposing .sync on the Wave at all - closure_call_sync's own contract
only returns undefined for an unsupported argument type before the closure
ever runs (a wave's argument is always a plain number, always supported),
so the probe is free unless the closure genuinely has a .sync twin, in
which case its result is reused as the Wave's first real sample instead of
being thrown away. If .sync isn't available, the Wave silently stays on the
existing async path - no crash, matches every other Sound consumer's
existing async fallback.
Contributor
Author
|
Contributor
Author
|
@martin-henz - found a real py2js limitation while testing this: a student callback passed into a module (e.g. stacking_adsr's envelope lambdas) can't call any other module function from inside it, since py2js marks every module closure asyncOnly no matter what. Not something we can fix on the modules side, filed it here: source-academy/py-slang#348 |
…preservation Martin: the module-evaluator bridge isn't obligated to be identity-preserving - two JS functions that are === may be represented by two Python functions that aren't 'is' to each other, and that's fine as a general FFI property. The earlier comment claimed the wave/closure caches make get_wave(s) == get_left_wave(s) a reliable invariant - they don't, since each engine's own moduleToPython still builds a fresh Python-side wrapper per conversion regardless of what's cached on the Conductor side. The caches stay (still real, just for avoiding redundant closure_make calls), the comment now says what they actually guarantee.
…comments Per Martin: avoid 'identical' when describing the two channels of a mono Sound in specs - say left_wave(t) == right_wave(t) for all t instead of claiming the two waves are the same object/reference. Reworded the public- facing docblocks (index.ts's and functions.ts's @module comments, the Sound type's own doc in types.ts, get_wave/record's JSDoc) to describe the behavioral contract a cadet program can actually observe, rather than an internal reference-equality detail. Where a comment is genuinely about this file's own implementation choice (make_sound assigning one Wave object to both fields, and later code taking advantage of that via leftWave === rightWave checks), left those as-is and called out explicitly that it's an implementation detail, not something the Sound Discipline promises.
Member
Good call: document properly and move on. Will explore the details. |
Akshay-2007-1
added a commit
that referenced
this pull request
Jul 24, 2026
#796's Conductor migration removed AudioPlayed/SoundModuleState entirely (the old js-slang moduleContexts.sound.state pattern doesn't apply to Conductor-based modules) and reshaped Sound from a Pair to {leftWave, rightWave, duration} - several doc pages' embedded, type-checked code samples still referenced the old shapes, failing the docs build's twoslash validation: - 2-bundle/4-conventions/3-errors.md: make_sound sample used pair() from js-slang/dist/stdlib/list against the old Sound type - rewritten to return the real {leftWave, rightWave, duration} shape. - 5-advanced/context.md, 3-tabs/1-overview.md, 3-tabs/3-editing.md: all three used sound as their example bundle for the general js-slang module context / getModuleState pattern, referencing AudioPlayed/SoundModuleState which no longer exist at all. Swapped to curve, which still legitimately uses this pattern (sound moved to Conductor's own channel-based state instead) - reused curve's actual CurveModuleState/drawnCurves shape and the real Curve tab's own getModuleState usage as the reference. Verified the two rewritten make_sound/context samples against a real tsc --strict run (not just eyeballing) - both type-check clean. 4-testing/4-unit/3-mock.md and 2-bundle/1-overview/1-overview.md also reference bundle-sound but only via imports/calls that stay valid regardless of Sound's internal shape (no destructuring assuming the old pair shape) - left as-is, not build-blocking, though mock.md's AudioContext example is now semantically stale (play() no longer touches AudioContext directly) and could use a follow-up pass.
Akshay-2007-1
added a commit
that referenced
this pull request
Jul 24, 2026
* sound: fix lint errors - EvaluatorParameterTypeError gap and a plain Error @sourceacademy/throw-runtime-error doesn't yet recognize EvaluatorParameterTypeError as assignable to RuntimeSourceError (same known gap already worked around in midi) - added the same eslint-disable + explanatory comment at each of the 6 call sites in functions.ts/index.ts. conductorToSound's invalid() helper was throwing a plain Error, not a Conductor error type at all - a real bug, not just a lint gap, since it would bypass proper student-facing error formatting. Fixed to EvaluatorRuntimeError; inlined the three call sites since the indirection through a helper function was itself what kept the linter from recognizing a type it accepts fine when thrown directly. The constructor's soundChannel wiring guard is left as a plain Error with a disable comment - a genuine internal precondition (Conductor host failed to provide the channel), never reachable from student code. Also fixed two @stylistic/member-delimiter-style warnings in recording.test.ts (semicolons vs commas in an inline type). * docs: fix stale sound-module type references breaking the docs build #796's Conductor migration removed AudioPlayed/SoundModuleState entirely (the old js-slang moduleContexts.sound.state pattern doesn't apply to Conductor-based modules) and reshaped Sound from a Pair to {leftWave, rightWave, duration} - several doc pages' embedded, type-checked code samples still referenced the old shapes, failing the docs build's twoslash validation: - 2-bundle/4-conventions/3-errors.md: make_sound sample used pair() from js-slang/dist/stdlib/list against the old Sound type - rewritten to return the real {leftWave, rightWave, duration} shape. - 5-advanced/context.md, 3-tabs/1-overview.md, 3-tabs/3-editing.md: all three used sound as their example bundle for the general js-slang module context / getModuleState pattern, referencing AudioPlayed/SoundModuleState which no longer exist at all. Swapped to curve, which still legitimately uses this pattern (sound moved to Conductor's own channel-based state instead) - reused curve's actual CurveModuleState/drawnCurves shape and the real Curve tab's own getModuleState usage as the reference. Verified the two rewritten make_sound/context samples against a real tsc --strict run (not just eyeballing) - both type-check clean. 4-testing/4-unit/3-mock.md and 2-bundle/1-overview/1-overview.md also reference bundle-sound but only via imports/calls that stay valid regardless of Sound's internal shape (no destructuring assuming the old pair shape) - left as-is, not build-blocking, though mock.md's AudioContext example is now semantically stale (play() no longer touches AudioContext directly) and could use a follow-up pass.
This was referenced Jul 26, 2026
martin-henz
added a commit
that referenced
this pull request
Jul 31, 2026
* Add CI/CD workflow to deploy conductor modules to modules-conductor repo
Deploys build artifacts from the conductor-migration branch to
source-academy/modules-conductor via GitHub Pages, allowing the
conductor version of modules to coexist with the current deployment.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Initial Conductor Migration (Repeat Module Migration, Testing, Documentation) (#698)
* feat(repeat): migrate initial repeat module
* feat(conductor): fix conductor documentation
* chore: add testing plugin
* fix: add chaining for undefined blockTags
* fix: make changes as per review
* Apply suggestions from code review
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* Migrate binary_tree module to Conductor (#790)
* Migrate binary_tree module to Conductor
Rewrites the binary_tree bundle as a Conductor BaseModulePlugin, backed
by IDataHandler pair/list primitives instead of js-slang's stdlib. Tree
entries are stored as OPAQUE values at the module boundary; is_tree and
is_empty_tree declare no arg type so they can accept any DataType and
answer false rather than throw, matching their predicate semantics.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address review feedback: null guards and EvaluatorRuntimeError
is_tree/is_empty_tree/assertNonEmptyTree now guard against a
missing/undefined value instead of crashing with a raw TypeError, and
the plugin's arity-check throws now use EvaluatorRuntimeError (already
used elsewhere in this module) instead of a generic Error.
Also adds the same __bindExportedMethods() workaround repeat/rune use
for the unbound-method issue in BaseModulePlugin.initialise()
(conductor#41), since this fix isn't merged upstream yet.
* Validate left/right are trees in make_tree
master's binary_tree gained this validation while conductor-migration
was diverging (make_tree(0, 0, null) previously constructed a
malformed tree silently instead of throwing). Ports the same check,
using EvaluatorTypeError to match this module's existing Conductor
error style rather than modules-lib's InvalidParameterTypeError, which
doesn't apply here since this module no longer goes through js-slang.
* binary_tree: remove __bindExportedMethods workaround
The upstream binding fix (source-academy/conductor#41) is merged and
published, making the per-module bind workaround unnecessary. Also drops the
constructor override, which was left as a pure passthrough to the base class
once the workaround was removed.
* fix: pin @sourceacademy/conductor to a real published version everywhere
repeat and testplugin depended on conductor via a bare, unpinned GitHub URL.
yarn.lock had resolved and locked that to a commit from before even the
BaseModulePlugin binding fix (conductor#41) - a real `yarn install` builds
against that stale, broken conductor entirely silently, since nothing
forces Yarn to re-resolve an already-locked git dependency. Other bundles
(rune, etc.) already depend on the versioned npm release; switched these
two to match (^0.7.0), which also resolves a duplicate-package TS error
that showed up in any bundle depending on both specs simultaneously.
* binary_tree: pin @sourceacademy/conductor to a published version
Was depending on conductor via a bare, unpinned GitHub URL, which yarn.lock
had resolved and locked to a commit from before the BaseModulePlugin binding
fix (conductor#41). Switched to the versioned npm range other bundles
already use (^0.7.0), matching the same fix applied to midi.
* binary_tree: add override modifiers required by conductor-migration's stricter tsconfig
Picked up as part of merging conductor-migration in - noImplicitOverride is
now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin.
* lock file fixed
* Fix lint errors blocking pre-push: unnecessary type assertions and Conductor error allowlist
- lib/buildtools, lib/testplugin: drop assertions that no longer change
the expression's type (same pattern as master's a2f369edd fix).
- eslint.config.js: allowlist EvaluatorTypeError/EvaluatorRuntimeError in
@sourceacademy/throw-runtime-error — they're Conductor's own
protocol-level error hierarchy, unrelated to js-slang's
RuntimeSourceError that the rule checks for. binary_tree is the first
Conductor-based bundle to throw these.
- binary_tree test: drop a now-unnecessary type assertion.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: henz <henz@comp.nus.edu.sg>
* Migrate Rune Module (#765)
* feat: migrate rune
* fix: update hollusion canvas to use promise based loading
* fix: restore curve's gl-matrix
* fix: make fixes to rune and repeat
* feat: add type safety for attachModuleMethod
* feat: add custom tags
* chore: lint files
* chore: make Lee Yi's changes
* fix: post-merge fallout from conductor-migration merge
- rune bundle index.ts: add override modifiers required by the
BaseModulePlugin signature in conductor 0.7.1 (this branch was on
0.6.0 before the merge's dependency consolidation), fix implicit-any
indexing when dynamically forwarding Rune-typed static fields, and
throw GeneralRuntimeError instead of a plain Error to satisfy the
throw-runtime-error lint rule.
- Rune tab index.tsx: this file wasn't touched by the merge conflict
(only hollusion_canvas.tsx and Rune.test.tsx were), so it still
referenced the pre-rename AnaglyphRune/HollusionRune/NormalRune
classes that conductor-migration renamed to Drawn*Rune. Updated all
usages and switched the instanceof check to the isHollusionRune type
guard the bundle already exports for this purpose.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* chore: bump conductor version
* chore: add type documentation for module method attachments
* chore: add type tests
* fix: do not unregister tab on destroy
---------
Co-authored-by: Akshay-2007-1 <akshayvemulapalli2007@gmail.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: henz <henz@comp.nus.edu.sg>
* buildtools: register @publicType/@publicReturnType as known Typedoc block tags
Typedoc warned "Encountered an unknown block tag" for every @publicType/
@publicReturnType JSDoc tag since they were never registered, and CI runs
buildtools build docs with --ci (errorOnWarning), turning those warnings
into a hard failure. rune is the first bundle using these tags in real
source (repeat only exercised them in tests), so this only surfaced now.
* docs: fix 5-animations.md twoslash samples broken by rune's Conductor migration
- HollusionCanvas sample: DrawnHollusionRune.draw() is async, await it before
assigning/calling the returned render function.
- Error Handling sample: animate_rune is now a RuneModulePlugin method, not a
free export, so the import no longer type-checks as written. Widen the
twoslash directive to the errors Typescript now actually reports (2614, 7006)
instead of the stale @noErrors: 2322 from before the migration.
* Revert @functionDeclaration/@variableDeclaration decorator reimplementation
Aarav caught this: 659ea90f7 ("feat: add custom tags") deliberately removed
declarations.ts and its decorator-based type inference in favour of the
@publicType/@publicReturnType JSDoc tags normalizeSignature() already
handles. That commit never cleaned up conductor.test.ts and
fixtures/conductorDeclarations.ts, which were written earlier (cb84fefcc)
against the old decorator system - so they were orphaned, not unfinished,
when I found them failing and reimplemented the removed feature.
- utils.ts: drop the source-file/decorator parsing helpers and their use in
cloneParameter/copyPluginSignature/copyPluginVariable/isExportedVariable;
restored to match how rune's real @publicType/@publicReturnType tags are
already handled by normalisation.ts, untouched by any of this.
- conductor.test.ts: delete the test exercising the removed decorator
system; rename the rune-bundle test to reflect what it actually verifies
(@publicType/@publicReturnType tags, which pass without any of the above).
- delete the now-fully-unused fixtures/conductorDeclarations.ts.
Kept: the initTypedocForJson outDir arg fix, and the isConductorReference
qualifiedName-matching fix (traces back to 7c75a4083, predates and is
unrelated to the decorator removal).
* Migrate scrabble module to Conductor (#792)
* Migrate scrabble module to Conductor
scrabble exports four static word/letter lists rather than functions,
so there's nothing for the usual exportedNames/@moduleMethod closure
path to wrap. Exposes them via DataType.OPAQUE (opaque_make in an
initialise() override) instead of DataType.ARRAY, since array_make has
no bulk constructor and the full word list is 172,820 entries.
Also drops the two full-array snapshot tests (scrabble_words/
scrabble_letters) - snapshotting 172,820 entries produced a
multi-million-line .snap file that hung vitest's serializer. The
existing index spot-checks already cover the full arrays; only the
~1,728-entry _tiny variants are snapshotted now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Run opaque_make calls concurrently in scrabble initialise()
The four exports are independent, so awaiting them one at a time added
needless latency. Addresses gemini-code-assist review comment on #792.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Switch scrabble's word lists from OPAQUE to real DataType.ARRAY
array_make's lack of a bulk constructor (one array_set call per
element) looked prohibitive for 172,820 words, so this used
opaque_make instead. That was never measured, and the actual cost is
~246ms one-time (TestDataHandler, same-thread as the evaluator - no
postMessage boundary between a module and its evaluator) for both
scrabble_words and the nested scrabble_letters. Cheap enough that
there's no reason to give up real indexing/print/iteration for it.
Follows from source-academy/py-slang#217's module-interop fixes and
the team's decision that Python lists/JS arrays should be the only
built-in data structure modules hand back - opaque_make stays reserved
for genuinely opaque payloads (e.g. binary_tree's node values), not
plain collections.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Shrey Jain <“shreyjain5132@email.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: pin @sourceacademy/conductor to a published version everywhere (#795)
* fix: pin @sourceacademy/conductor to a real published version everywhere
repeat and testplugin depended on conductor via a bare, unpinned GitHub URL.
yarn.lock had resolved and locked that to a commit from before even the
BaseModulePlugin binding fix (conductor#41) - a real `yarn install` builds
against that stale, broken conductor entirely silently, since nothing
forces Yarn to re-resolve an already-locked git dependency. Other bundles
(rune, etc.) already depend on the versioned npm release; switched these
two to match (^0.7.0), which also resolves a duplicate-package TS error
that showed up in any bundle depending on both specs simultaneously.
* fix: address review feedback on conductor pin PR
- Add @sourceacademy/conductor to npmPreapprovedPackages to bypass the age gate
- Reset yarn.lock to base state and rerun yarn install so only conductor's resolution changes, undoing unintentional dependency downgrades
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: pin vite to a single resolved version workspace-wide
vitest's own dependency on vite ("^6.0.0 || ^7.0.0 || ^8.0.0") isn't
constrained to match the version hoisted for direct consumers
(lib/buildtools, lib/repotools), so Yarn can hoist a second, older
nested copy depending on install order. When that happens,
loadConfigFromFile (imported from the bare "vite" package) and
vitest/config's ViteUserConfig type augmentation are built against
two structurally different UserConfig types, breaking lib/repotools'
tsc with TS2339/TS2321 errors.
Reported by Prof Martin Henz while testing PR #791 locally.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: scope the vite pin from #795 to vitest only (#811)
The blanket `resolutions: { "vite": "^8.1.0" }` from #795 forced
*every* vite consumer in the workspace onto 8.1.0, including
vitepress@1.6.4 (used by the docs/docserver build), which has a
hard, non-floating dependency on vite@^5.4.14 and is not compatible
with Vite 8 internally. That broke the docserver and plotly builds
on the very next install after #795 merged.
Scope the resolution to `vitest/vite` so it only overrides vite as
resolved through vitest's own dependency graph (the actual source of
the duplicate-copy TS error), leaving vitepress's separate vite@5
tree untouched.
* fix: guard against undefined children in typedoc-plugin's buildJson (#812)
reflection.children! assumed every documented module has at least one
child reflection. That breaks for scrabble: after the conductor doc
pipeline strips its plugin class, there's nothing left to promote,
since all four of scrabble's exports (scrabble_words, etc.) are built
entirely at runtime via this.exports.push(...) rather than as
statically-analyzable class methods/properties. TypeDoc then leaves
children undefined instead of [], and the reduce() over it throws
"Cannot read properties of undefined (reading 'reduce')", which broke
build:docs (and therefore the conductor-migration -> modules-conductor
deploy) for the whole workspace as soon as #792 (scrabble) merged.
* Migrate midi module to Conductor (#791)
* Migrate midi module to Conductor
Splits midi into a pure, evaluator-free functions.ts (unchanged
signatures) and a Conductor-facing index.ts plugin, since sound and
stereo_sound import midi_note_to_frequency and friends directly as
plain TypeScript and sound/stereo_sound's own Source-facing APIs
re-export several of these functions. Migrating index.ts's exports to
require an IDataHandler would have broken both call sites immediately.
- functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic
- conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor
list conversion, accidental validation) used by the plugin; kept
separate from index.ts so they stay importable from vitest, which
hits a decorator syntax error importing index.ts directly
- index.ts: BaseModulePlugin subclass wrapping the pure functions;
SHARP/FLAT/NATURAL are pushed onto `exports` directly in the
constructor since BaseModulePlugin.initialise() only registers
exportedNames that are functions
- Includes the same __bindExportedMethods() workaround as
repeat/rune/binary_tree for the unbound-method issue in
BaseModulePlugin.initialise() (source-academy/conductor#41)
- sound/stereo_sound's functions.ts and index.ts updated to import
midi's pure functions from the new `/functions` subpath instead of
the bundle root, which now exports the Conductor plugin
* Address review feedback: use string literals instead of .name
Function.prototype.name gets mangled under minification, which would
turn these error messages into cryptic garbage like "t expects...".
Two of these were carried over unchanged from the original module; the
third is in the new Conductor-facing index.ts.
* Port midi's master-only functions and validation
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
* midi: adapt to conductor's options-object assertNumberWithinRange signature
* midi: fix scales' runtime js-slang dependency and address review feedback
scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is
unavailable under Conductor's module loader (the require() shim it's given is
a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for
any scale function. Build the intermediate list with a plain array tuple
instead, since Pair<H, T> is just [H, T] structurally.
Also addresses Aarav's review comments on #791:
- Removes __bindExportedMethods now that the upstream binding fix
(source-academy/conductor#41) is merged and published.
- Widens letter_name_to_midi_note/midi_note_to_letter_name/
letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/
get_accidental/key_signature_to_key's note/accidental parameters to plain
string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in
index.ts. Doing this exposed two real validation gaps that the casts had
been silently papering over: add_octave_to_note never validated `note` at
all (just interpolated it into the result string), and
midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP
accidental as FLAT instead of rejecting it. Both now throw
EvaluatorParameterTypeError for invalid input, with regression tests added.
* midi: drop unnecessary js-slang runtime dependency, fix real dependency resolution
scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type
shape, not js-slang itself. Replaced with a local Scale type; js-slang moves
to devDependencies since only the test suite still uses it.
Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub
URL, which yarn.lock had resolved and locked to a commit from before even the
BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using
a local portal: link for testing) was building against that stale, broken
conductor entirely silently. Switched to the same versioned npm range other
bundles already use (^0.7.0), and reverted the two calls to
assertNumberWithinRange that had been written against conductor PR #43's
still-unpublished options-object signature back to the options actually
published in 0.7.0.
* midi: add override modifiers and fix scale test type error
Picked up as part of merging conductor-migration in - noImplicitOverride
is now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin. Also fixed a test that built its input
scale via js-slang's untyped list() instead of midi's own js-slang-free
Scale shape, which only surfaced as a real tsc error once the merge
brought stricter checking back online.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: align @sourceacademy/conductor pin with catalog: convention
Matches the same fix already applied on #795 and #796: adds
@sourceacademy/conductor to the yarn catalog and
npmPreapprovedPackages, and switches midi's own package.json to
"catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't
leave midi on the old convention once #795 lands.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: reuse parseNoteWithOctave as the canonical validator in add_octave_to_note
Addresses CodeRabbit's review comment on PR #796 (surfaced there via
shared branch history with sound, but the finding is midi's own):
add_octave_to_note's inline regex accepted note spellings that
noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb -
accidentals that don't exist for those note names), and let lowercase
input escape unnormalized through a type assertion. Now validates via
parseNoteWithOctave (rejecting digits upfront, since that function
alone would accept an octave already being present) and reconstructs
using the normalized note name, preserving the original accidental
spelling exactly as given.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Pin vite to one version workspace-wide, fixing a local yarn install failure
Prof Martin hit a build failure running yarn install locally: lib/repotools'
own tsc failed with "Property 'test' does not exist on type 'UserConfig'"
and "Excessive stack depth comparing types 'UserConfig' and 'UserConfig'"
in src/testing/index.ts.
Root cause: two different resolved copies of vite existed side by side -
the workspace root hoists vite@8.1.4 (requested directly by lib/buildtools
and devserver, both "^8.1.0"), but vitest@4.1.9's own internal dependency
on vite resolved its own nested copy at 8.0.12 instead, since nothing
constrained it to match. src/testing/index.ts imports loadConfigFromFile
from the bare "vite" package (resolving to the hoisted 8.1.4) while
vitest/config's ViteUserConfig type augmentation is built against vitest's
own nested 8.0.12 UserConfig - TypeScript sees these as two structurally
different types instead of the same one, hence the errors. Reported as
non-deterministic since it depends on exactly how Yarn happens to hoist
things on a given install.
Fixed with a resolutions entry pinning every resolution of vite to
^8.1.0 workspace-wide (matching Prof Martin's own suggested fix) - the
nested copy under vitest/node_modules is now gone entirely after
reinstalling, both consumers resolve the identical module, and
lib/repotools' tsc (and lib/buildtools', the other direct vite consumer)
are both clean.
* Add real JSDoc to index.ts's wrapper methods so docs actually regenerate
Mirrors rune's pattern (#765): the doc generator reads JSDoc off the
@moduleMethod-decorated wrapper methods in index.ts, not off the real
implementations in functions.ts/scales.ts they delegate to - a thin
wrapper with no comment of its own means the generated docs.json comes
back "No description available" for every export, even though the real
JSDoc is sitting right there on the underlying function.
Copied each wrapper's description/@param/@returns/@example from its
functions.ts/scales.ts counterpart. No @publicType/@publicReturnType
overrides needed here (unlike rune's Rune/OPAQUE case) - every one of
midi's parameter and return types (NUMBER, CONST_STRING, BOOLEAN, LIST)
already has an unambiguous native mapping.
Verified by rebuilding docs: all 19 exports now show real descriptions
instead of "No description available" (confirmed per-export, not just
absence of warnings). tsc/lint/test (33/33) all clean.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: regenerate yarn.lock, drifted out of sync after midi (#791) merge
yarn install --immutable was failing on conductor-migration ("The lockfile
would have been modified by this install") since the midi merge, breaking
the deploy-to-modules-conductor pipeline at the Install Dependencies step.
The committed lockfile was missing a batch of esbuild@0.21.5 optional
platform-dependency entries that a fresh install adds back. Regenerated
via a plain yarn install; no dependency version changes, just filling in
the entries the merge left out.
* Migrate Sound Module to Conductor (#796)
* Migrate midi module to Conductor
Splits midi into a pure, evaluator-free functions.ts (unchanged
signatures) and a Conductor-facing index.ts plugin, since sound and
stereo_sound import midi_note_to_frequency and friends directly as
plain TypeScript and sound/stereo_sound's own Source-facing APIs
re-export several of these functions. Migrating index.ts's exports to
require an IDataHandler would have broken both call sites immediately.
- functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic
- conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor
list conversion, accidental validation) used by the plugin; kept
separate from index.ts so they stay importable from vitest, which
hits a decorator syntax error importing index.ts directly
- index.ts: BaseModulePlugin subclass wrapping the pure functions;
SHARP/FLAT/NATURAL are pushed onto `exports` directly in the
constructor since BaseModulePlugin.initialise() only registers
exportedNames that are functions
- Includes the same __bindExportedMethods() workaround as
repeat/rune/binary_tree for the unbound-method issue in
BaseModulePlugin.initialise() (source-academy/conductor#41)
- sound/stereo_sound's functions.ts and index.ts updated to import
midi's pure functions from the new `/functions` subpath instead of
the bundle root, which now exports the Conductor plugin
* Address review feedback: use string literals instead of .name
Function.prototype.name gets mangled under minification, which would
turn these error messages into cryptic garbage like "t expects...".
Two of these were carried over unchanged from the original module; the
third is in the new Conductor-facing index.ts.
* Port midi's master-only functions and validation
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
* midi: adapt to conductor's options-object assertNumberWithinRange signature
* midi: fix scales' runtime js-slang dependency and address review feedback
scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is
unavailable under Conductor's module loader (the require() shim it's given is
a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for
any scale function. Build the intermediate list with a plain array tuple
instead, since Pair<H, T> is just [H, T] structurally.
Also addresses Aarav's review comments on #791:
- Removes __bindExportedMethods now that the upstream binding fix
(source-academy/conductor#41) is merged and published.
- Widens letter_name_to_midi_note/midi_note_to_letter_name/
letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/
get_accidental/key_signature_to_key's note/accidental parameters to plain
string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in
index.ts. Doing this exposed two real validation gaps that the casts had
been silently papering over: add_octave_to_note never validated `note` at
all (just interpolated it into the result string), and
midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP
accidental as FLAT instead of rejecting it. Both now throw
EvaluatorParameterTypeError for invalid input, with regression tests added.
* midi: drop unnecessary js-slang runtime dependency, fix real dependency resolution
scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type
shape, not js-slang itself. Replaced with a local Scale type; js-slang moves
to devDependencies since only the test suite still uses it.
Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub
URL, which yarn.lock had resolved and locked to a commit from before even the
BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using
a local portal: link for testing) was building against that stale, broken
conductor entirely silently. Switched to the same versioned npm range other
bundles already use (^0.7.0), and reverted the two calls to
assertNumberWithinRange that had been written against conductor PR #43's
still-unpublished options-object signature back to the options actually
published in 0.7.0.
* midi: add override modifiers and fix scale test type error
Picked up as part of merging conductor-migration in - noImplicitOverride
is now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin. Also fixed a test that built its input
scale via js-slang's untyped list() instead of midi's own js-slang-free
Scale shape, which only surfaced as a real tsc error once the merge
brought stricter checking back online.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: rebuild playback/recording on a self-contained Conductor channel
Scraps the earlier plugins-repo sound-io plugin pair in favor of the
pattern confirmed against rune's migration (PR #765): SoundModulePlugin
declares its own channelAttach directly, and modules/src/tabs/Sound
implements IPlugin/SoundTabRpc as the host-side counterpart, talking
over Conductor's makeRpc helper - no separate runner/web plugin
package needed at all.
Also fixes plotly's draw_sound_2d, the one other consumer of
bundle-sound's types, for the Wave type's redesign from a plain
(t: number) => number to an async generator (so that stepping and
nested user closures evaluate correctly on the CSE machine).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: unify with stereo_sound - one module, Sound is always stereo
Retires stereo_sound as a separate bundle/tab entirely. Sound becomes
{leftWave, rightWave, duration} always; "mono" isn't a separate type,
it's just the common case where leftWave === rightWave (same
reference), produced by make_sound. make_stereo_sound builds a
genuinely stereo Sound from two different waves. Every combinator
(consecutively/simultaneously/adsr/phase_mod/stacking_adsr/
instruments) now operates on this one shape via small per-channel
helpers (joinWaves/sumWaves/adsrWave/phaseModWave), so composing a
"mono" sound with a stereo one is a non-issue - there's nothing to
convert, and none of stereo_sound's oscillator/envelope math is
duplicated a second time the way it was as a separate bundle.
get_wave/get_left_wave/get_right_wave: get_wave keeps meaning "the"
wave (== left channel), so existing SICP-style code (play(sine_sound(...)),
get_wave(sound)) keeps working unchanged for the common mono case.
Adds a lower Wave-returning layer alongside the existing Sound layer
(sine_wave/square_wave/triangle_wave/sawtooth_wave/noise_wave/
silence_wave), with sine_sound etc. as thin convenience wrappers
(sine_sound = (freq, dur) => make_sound(sine_wave(freq), dur)).
record()/record_for() use however many channels the input device
actually has - a mono microphone (the common case) produces a Sound
whose left and right channels are the same wave; no separate
record_stereo. play/samplesToSound skip redoing work for a mono
Sound (sampled once, not twice) by checking leftWave === rightWave.
New stereo-specific operations: make_stereo_sound, play_waves,
pan, pan_mod, squash, get_left_wave, get_right_wave.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: rebuild Sound/functions/index/tab for the sound+stereo_sound unification
Continuation of the previous commit (which only picked up the
stereo_sound/StereoSound deletions due to a failed multi-pathspec git
add) - this is the actual Sound={leftWave,rightWave,duration} rework
of the sound bundle and its tab described there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* align @sourceacademy/conductor pins with PR #795's catalog: convention
#795 (fix/conductor-dependency-pin) is about to merge into
conductor-migration first, establishing @sourceacademy/conductor in
the yarn catalog and npmPreapprovedPackages. Switching midi/sound/
tab-Sound's package.json over to "catalog:" now (matching #795
verbatim) avoids re-doing this exact reconciliation the next time
this branch merges conductor-migration in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: fix O(N) nested yield* delegation in consecutively/simultaneously
Addresses Gemini's high-priority review comments on PR #796: reduce()
was nesting joinWaves/sumWaves closures sounds.length deep, so
sampling near the end of a long chain meant up to N nested async
generator delegations per sample at 44100 Hz - real overhead for
async generators that plain synchronous functions (the pre-migration
implementation) never had. Rewritten as a flat scan that picks the
active sound directly and yield*s into it once, keeping delegation
depth O(1) regardless of how many sounds are combined, while still
threading through user closures correctly.
Also fixes a real (if narrow) NaN: linear_decay(0) computed 1 - 0/0
when release_ratio (or a future caller with decay_ratio) is exactly
0, corrupting the sample at that instant. Sample rate/instrument
functions never happened to hit it because of how the surrounding
branch conditions are structured, but adsr's release branch can.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: align @sourceacademy/conductor pin with catalog: convention
Matches the same fix already applied on #795 and #796: adds
@sourceacademy/conductor to the yarn catalog and
npmPreapprovedPackages, and switches midi's own package.json to
"catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't
leave midi on the old convention once #795 lands.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: address CodeRabbit review findings on PR #796
- validateDuration now rejects NaN/Infinity, not just negatives
(both used to slip through and crash sampleWave's Float32Array
allocation with a raw RangeError instead of a clean module error).
- closureToWave validates the closure's result is actually a number
before returning it - a student-supplied wave returning e.g. a
string previously corrupted playback silently via `as number`.
- stacking_adsr validates each envelope-list element is a closure
before invoking it, for the same reason.
- consecutively/simultaneously/adsr/phase_mod/conductorToSound all
preserve the "mono means leftWave === rightWave" invariant again -
building both channels independently (even from identical inputs)
produced two behaviourally-identical-but-distinct waves, silently
doubling sampling work and losing the fast path in play()/etc.
- play()/stop() guard against a stale playSamples() completion
clobbering a newer play()'s isPlaying state via a generation token
(stop-A/start-B/late-settle-A ordering).
- tab: requestMicPermission() disposes the previous MediaStream
before re-requesting, so a denied re-request can't leave stale
tracks running and reusable by startRecording().
- tab: startRecording() now actually awaits MediaRecorder's start
event (and rejects on error) instead of resolving as soon as
.start() returns, matching the SoundTabRpc contract.
Not addressed (flagged as false positive or out of scope for a
quick fix, not silently dropped):
- The module doc's "a wave returns a number" description is correct
as written - it's the student-facing Source-language contract, not
the internal async-generator implementation detail (which is
already documented separately on the Wave type in types.ts).
- Overlapping/concurrent recording sessions (record/record_for only
gate on isPlaying, not a dedicated recording-state flag) and
pan/pan_mod sampling the shared source/modulator wave twice per
channel are both real but non-trivial fixes requiring more
substantial state-tracking/sampling-order changes; left for a
follow-up rather than a rushed partial fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: cross-reference Wave's internal async-generator contract in the module doc
Partial concession on CodeRabbit's doc-comment finding: the primary
description stays as the Source-facing "number -> number" contract
(that's the actual, correct student experience - the CSE machine
threads the async-generator machinery transparently), but adds a
one-line pointer to where the internal TS contract is documented, for
maintainers reading this file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: reuse parseNoteWithOctave as the canonical validator in add_octave_to_note
Addresses CodeRabbit's review comment on PR #796 (surfaced there via
shared branch history with sound, but the finding is midi's own):
add_octave_to_note's inline regex accepted note spellings that
noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb -
accidentals that don't exist for those note names), and let lowercase
input escape unnormalized through a type assertion. Now validates via
parseNoteWithOctave (rejecting digits upfront, since that function
alone would accept an octave already being present) and reconstructs
using the normalized note name, preserving the original accidental
spelling exactly as given.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix playback/recording races and add ADSR validation, found via manual testing
Manually testing all three migrated modules (binary_tree, midi, sound)
together against a local frontend build surfaced several issues
specific to sound, now fixed:
- play() threw "Previous sound still playing!" on any overlapping or
looped call. Repeated/looped play() calls now queue and play one
after another instead (like consecutively, but built up call-by-call
rather than pre-combined into one Sound). stop() cancels anything
still queued behind the currently-playing sound, not just the
current one.
- The tab's "Constructing..." status (shown while play() samples a
Wave into a buffer - a duration-proportional step that happens
entirely before actual playback starts) never actually appeared,
since the notification was fire-and-forget and could race the tab's
own asynchronous loading. Now a real acknowledged RPC call, awaited
before sampling begins.
- init_record() was fire-and-forget: it kicked off the permission
request but returned immediately, so calling record()/record_for()
right after (the natural way to write it) could race the still-
pending permission grant and throw even though permission had
genuinely just been granted. init_record() now awaits the actual
permission result before returning.
- record()/record_for()'s returned "sound promise" threw "recording
still being processed" until called again later, requiring manual
polling. Both now return a promise that genuinely awaits the
recording finishing processing instead.
- adsr() had no validation that attack_ratio + decay_ratio +
release_ratio stays within 1, or that each ratio/sustain_level is a
finite number in [0, 1] - silently producing a discontinuous
envelope instead of an error. Added validation, with the actual
envelope-shaping logic split into an unvalidated adsrTransformer()
used internally by piano/violin/cello/trombone/bell, so validating
adsr() doesn't retroactively break trombone's second-harmonic
envelope (ratios summing to 1.0236, a quirk present since before
the Conductor migration - preserved rather than silently changed).
* Move playback sequencing to the host tab; fix premature AudioContext teardown
Found via extensive live testing against a local frontend build:
- The Worker running a program is terminated as soon as the script
finishes (conduit.terminate(), called after every Run to fix a
worker-leak). play() is intentionally fire-and-forget, so a script
can finish - and the Worker be killed - well before queued playback
has actually started or finished. Sequencing that playback via a
Worker-side queue (the previous design) meant anything still queued
when the Worker died simply never got its playSamples() RPC sent at
all.
- functions.ts' play() now dispatches its playSamples() call
immediately once sampling finishes, instead of waiting its turn in
a local queue - so the RPC always gets sent before the Worker can be
torn down. Actual sequencing (so playback doesn't overlap) moves to
SoundTabPlugin on the host side, which outlives the Worker: it now
owns its own playback queue and a stop-generation counter so a
still-queued call correctly gets cancelled by stop().
- SoundTabPlugin.destroy() (called on every Run's teardown) no longer
closes the AudioContext or unregisters the tab immediately - both
are deferred until whatever's playing (or still queued) finishes
naturally, tracked via a dedicated pending-playback counter rather
than activeSources.size, which hits 0 momentarily between any one
sound ending and the next queued one starting and was otherwise
misread as "everything is done," silently killing the AudioContext
mid-queue.
- notifyConstructing()/playSamples() status updates are now
recomputed from combined constructing+active-source state instead
of set unconditionally, so an earlier sound finishing can't clobber
a later sound's still-in-flight 'constructing' status.
* sound: fast synchronous path for module-native waves, skip per-sample async overhead
Wave is AsyncGenerator-based so a wave wrapping a user-supplied Conductor closure
(closureToWave, in index.ts) can be driven through evaluator.closure_call_unchecked -
itself an AsyncGenerator, since it steps the calling evaluator's CSE machine. But
every wave built entirely from module-native math (oscillators, envelopes,
instruments, and every combinator) got the same generator wrapper even though it
never actually yields - and every AsyncGenerator#next() resolves via microtask
regardless of whether anything inside really awaits. sampleWave calls a wave once
per sample (44100 times per second of audio), so a multi-second Sound made of nothing
but built-in instruments was paying real async overhead - measured as the dominant
cost of play() for a ~21s cello passage - for zero actual asynchrony.
Adds Wave.sync: an optional plain (t: number) => number twin, present iff a wave
provably never needs to cross into user code. syncWave() builds both forms from one
computation. Every combinator (clipToDuration, consecutiveWave, simultaneousWave,
adsrWave, phaseModWave, gainWave, squash, panModAmountWave, pan_mod, interpolatedWave)
propagates sync when every wave feeding into it has one, and falls back to the
existing yield*-driven path the moment a closure-backed wave (which never sets sync)
is anywhere in the composition - so a student-supplied wave function's stepping/
breakpoint visibility, and Conductor's worker-boundary async contract, are entirely
unaffected. sampleWave itself takes the fast path whenever the wave it's sampling has
one.
Confirms the fast path is a pure performance change, not a semantic one: correctness
depends only on the fallback being exact, which the existing PAIR/ARRAY module-interop
and existing sound-bundle test suite (64 tests, all passing) already exercise the
composition shapes for.
* sound: sync fast path for student-authored wave closures, when the evaluator supports it
closureToWave wrapped every student-supplied wave function as a plain async
generator calling evaluator.closure_call_unchecked - correct, but paying a real
microtask (and a full evaluator round-trip) per sample even for the simplest
possible wave, sampled 44100x/sec. The built-in-wave fast path landed earlier in
this file's siblings (functions.ts) doesn't help here: a student's own wave has
to actually run inside whichever engine (CSE/PVML/py2js) is executing the
program - there's no way for the module itself to skip that call.
An engine can now opt a closure into a synchronous fast path by implementing
closure_call_sync (py-slang's GenericDataHandler, exposed so far only by py2js,
whose dual-compiled functions already have a synchronous body internally -
rt.callSync - that just wasn't reachable from outside py-slang before). This
module checks for the method generically - closureToWave attaches wave.sync
only when the evaluator actually provides closure_call_sync, and every other
engine (CSE, PVML, until they grow the same capability) is completely
unaffected: the method simply doesn't exist on their evaluator instance, so
every wave stays on the existing async path, unchanged.
The one correctness wrinkle worth being explicit about: closure_call_sync
returning undefined means "no sync form for this closure" - safe as a signal
before the closure has run, but from inside wave.sync (a plain function, not a
generator - there's no way to suspend and retry via the async path once we're
in here) a missing sync form after the fact is treated as a hard internal
error rather than silently producing a wrong sample.
* sound: waveToConductorClosure was silently dropping the sync fast path
closureToWave (Conductor closure -> internal Wave) already picks up wave.sync
from the evaluator's closure_call_sync when available, but the reverse
direction didn't: waveToConductorClosure (internal Wave -> Conductor closure)
always built a plain async-only wrapper, with no way for anything downstream
to know the original wave ever had a sync form. Since make_sound/
make_stereo_sound/get_wave/etc. all round-trip a Sound's waves through this
function to hand it back to Python, every Sound handed back to a student -
even one built entirely from a py2js closure that supports closure_call_sync -
permanently lost the fast path the instant it was constructed. play() on that
same Sound later would then find closure_call_sync returning undefined (no
.sync on the re-wrapped closure) and hit the "no synchronous form" internal
error closureToWave raises for exactly this shouldn't-happen case.
Fixed by having waveToConductorClosure attach the same kind of .sync twin
(reusing wave.sync, converting its number result to/from a TypedValue) before
handing the function to closure_make, mirroring closureToWave's direction
exactly. No behavior change for a wave with no sync form (CSE closures, or a
wave that genuinely needs a host round-trip) - conductorWave.sync is simply
never attached.
* sound: address review feedback on record/record_for and error messages
- record/record_for: stop hardcoding the function name in error messages
(record: ..., record_for: ...) - use `${record.name}`/`${record_for.name}`
like every other error in this file, so a rename can't silently go stale.
Same fix for play's EvaluatorParameterTypeError/duration-negative error,
which had the same hardcoded-literal bug.
- record/record_for: replace the nested setTimeout+.then() towers with a
single async IIFE using es-toolkit's delay(), preserving the exact same
event ordering (pre-recording-signal pause, recording signal, pre-recording
pause, recording, recording signal) but as flat sequential awaits.
* sound: accept DataType.ARRAY as equally valid to PAIR for Sound values
py-slang's module interface no longer has a distinct "pair" representation
(source-academy/py-slang#307): pythonToModule now builds every Python list/
pair as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A Sound
built here via soundToConductor and round-tripped out to Python (assigned to
a variable, passed to another module call like get_wave/is_sound/play) then
arrives tagged ARRAY, not PAIR - conductorToSound/is_sound hardcoded
`value.type !== DataType.PAIR` checks broke on this, same class of bug as
binary_tree's #813.
Fixed the same way: isPairLike(value) accepts either DataType.PAIR or
DataType.ARRAY wherever a Sound's own tag is checked - pair_head/pair_tail
already read either shape identically (py-slang's GenericDataHandler
bridge), so this is purely about validation, not traversal.
Also fixed conductorListToSounds (consecutively/simultaneously's list
argument) and stacking_adsr's envelopes list: a genuine Python list of any
length now crosses as a flat ARRAY too, not just a 2-element pair, so the
old PAIR-chain-only walk would silently return zero elements for a real
multi-sound list. Added readListElements to read either an ARRAY (via
array_length/array_get) or a PAIR/EMPTY_LIST chain, matching py-slang's own
GenericDataHandler.readListElements pattern.
soundToConductor's own construction (pair_make calls) is unchanged - it
always freshly builds a genuine PAIR; only validation of an incoming
(possibly round-tripped) value needed to widen.
* sound: don't crash when closure_call_sync exists but this closure has no .sync twin
closure_call_sync lives on GenericDataHandler, the shared IDataHandler
implementation across all of py-slang's engines - it's always present
regardless of which engine is actually running, so closureToWave's old check
("does the method exist") was never actually engine-specific despite its own
comment claiming otherwise. Only some py2js closures ever carry a real
.sync twin; CSE and PVML closures never do. Every student-authored wave
function played via play()/play_wave() on CSE or PVML was hitting the
'Internal error: closure_call_sync unexpectedly had no synchronous form'
throw on its very first sample, since .sync was attached unconditionally
whenever the method merely existed.
Fixed by determining sync-capability once, with a real probe call, before
ever exposing .sync on the Wave at all - closure_call_sync's own contract
only returns undefined for an unsupported argument type before the closure
ever runs (a wave's argument is always a plain number, always supported),
so the probe is free unless the closure genuinely has a .sync twin, in
which case its result is reused as the Wave's first real sample instead of
being thrown away. If .sync isn't available, the Wave silently stays on the
existing async path - no crash, matches every other Sound consumer's
existing async fallback.
* sound: correct closure-cache comment per Martin's ruling on identity preservation
Martin: the module-evaluator bridge isn't obligated to be identity-preserving
- two JS functions that are === may be represented by two Python functions
that aren't 'is' to each other, and that's fine as a general FFI property.
The earlier comment claimed the wave/closure caches make
get_wave(s) == get_left_wave(s) a reliable invariant - they don't, since
each engine's own moduleToPython still builds a fresh Python-side wrapper
per conversion regardless of what's cached on the Conductor side. The
caches stay (still real, just for avoiding redundant closure_make calls),
the comment now says what they actually guarantee.
* sound: describe mono behaviorally, not by reference identity, in doc comments
Per Martin: avoid 'identical' when describing the two channels of a mono
Sound in specs - say left_wave(t) == right_wave(t) for all t instead of
claiming the two waves are the same object/reference. Reworded the public-
facing docblocks (index.ts's and functions.ts's @module comments, the Sound
type's own doc in types.ts, get_wave/record's JSDoc) to describe the
behavioral contract a cadet program can actually observe, rather than an
internal reference-equality detail. Where a comment is genuinely about this
file's own implementation choice (make_sound assigning one Wave object to
both fields, and later code taking advantage of that via leftWave ===
rightWave checks), left those as-is and called out explicitly that it's an
implementation detail, not something the Sound Discipline promises.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* sound: fix lint errors on conductor-migration (#818)
* sound: fix lint errors - EvaluatorParameterTypeError gap and a plain Error
@sourceacademy/throw-runtime-error doesn't yet recognize
EvaluatorParameterTypeError as assignable to RuntimeSourceError (same known
gap already worked around in midi) - added the same eslint-disable +
explanatory comment at each of the 6 call sites in functions.ts/index.ts.
conductorToSound's invalid() helper was throwing a plain Error, not a
Conductor error type at all - a real bug, not just a lint gap, since it
would bypass proper student-facing error formatting. Fixed to
EvaluatorRuntimeError; inlined the three call sites since the indirection
through a helper function was itself what kept the linter from recognizing
a type it accepts fine when thrown directly.
The constructor's soundChannel wiring guard is left as a plain Error with a
disable comment - a genuine internal precondition (Conductor host failed to
provide the channel), never reachable from student code.
Also fixed two @stylistic/member-delimiter-style warnings in
recording.test.ts (semicolons vs commas in an inline type).
* docs: fix stale sound-module type references breaking the docs build
#796's Conductor migration removed AudioPlayed/SoundModuleState entirely
(the old js-slang moduleContexts.sound.state pattern doesn't apply to
Conductor-based modules) and reshaped Sound from a Pair to
{leftWave, rightWave, duration} - several doc pages' embedded, type-checked
code samples still referenced the old shapes, failing the docs build's
twoslash validation:
- 2-bundle/4-conventions/3-errors.md: make_sound sample used pair() from
js-slang/dist/stdlib/list against the old Sound type - rewritten to
return the real {leftWave, rightWave, duration} shape.
- 5-advanced/context.md, 3-tabs/1-overview.md, 3-tabs/3-editing.md: all
three used sound as their example bundle for the general js-slang module
context / getModuleState pattern, referencing AudioPlayed/SoundModuleState
which no longer exist at all. Swapped to curve, which still legitimately
uses this pattern (sound moved to Conductor's own channel-based state
instead) - reused curve's actual CurveModuleState/drawnCurves shape and
the real Curve tab's own getModuleState usage as the reference.
Verified the two rewritten make_sound/context samples against a real tsc
--strict run (not just eyeballing) - both type-check clean.
4-testing/4-unit/3-mock.md and 2-bundle/1-overview/1-overview.md also
reference bundle-sound but only via imports/calls that stay valid regardless
of Sound's internal shape (no destructuring assuming the old pair shape) -
left as-is, not build-blocking, though mock.md's AudioContext example is now
semantically stale (play() no longer touches AudioContext directly) and
could use a follow-up pass.
* docs: fix CurveDrawn not being an exported type from bundle-curve/types (#819)
CurveDrawn is imported into curve/types.ts from ./curves_webgl but never
re-exported from it - only CurveModuleState (which uses CurveDrawn as part
of its drawnCurves field type) is actually exported from
@sourceacademy/bundle-curve/types. #818's context.md fix imported CurveDrawn
directly, which doesn't exist at that import path (TS2459), breaking the
docs build again. Fixed by deriving the element type via
CurveModuleState['drawnCurves'] instead - the same pattern already used
correctly in this PR's other two fixed files (3-tabs/1-overview.md,
3-tabs/3-editing.md), just missed here.
Verified against a real tsc --strict run this time, not just eyeballing -
all imports across all 4 previously-touched doc pages checked against their
actual compiled .d.ts exports, not just what's declared in bundle source.
* Fix predicate methods declaring 0 arity instead of DataType.ANY (#821)
is_sound (sound), is_tree/is_empty_tree (binary_tree), and
is_note_with_octave (midi) are @moduleMethod-decorated predicates meant to
accept one value of any Conductor DataType and answer false rather than
throw. They were declared with an empty args array ([]), which correctly
signals "no fixed type" but incorrectly reports arity 0 via
closure_arity() - the array's length is the only thing engines can read
back as "how many parameters this closure takes."
Every engine reads this same signature (GenericDataHandler.closure_arity),
but only py2js treats it as an exact contract, so a real call like
is_sound(s) threw "is_sound() takes 0 arguments but 1 was given" there
while CSE and PVML worked fine. Found while debugging that report.
DataType.ANY exists in conductor precisely for "one argument, unrestricted
type" - using it instead of [] reports the correct arity (1) without
changing any runtime behavior (GenericDataHandler never type-checks args
against the declared signature, only counts them). Requires bumping the
conductor catalog range to ^0.7.2 (ANY was added in 0.7.1; 0.7.0 predates
it) and normalizing binary_tree's package.json off a hardcoded
"^0.7.0" onto the shared "catalog:" entry, which was silently out of step
with every other bundle.
Claude-Session: https://claude.ai/code/session_01KrwJGug3rCXijRseRjys9V
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Bump @sourceacademy/conductor catalog pin to 0.8.0 (#822)
PR source-academy/conductor#53 makes hostLoadPlugin/requestLoadPlugin
async (Promise<void> instead of void) and renames the RPC procedure
from the notification $requestLoadPlugin to the awaitable
requestLoadPlugin.
No module in this repo calls hostLoadPlugin/requestLoadPlugin directly
(only IModulePlugin/type-level conductor surface is used), so this is
purely a version bump. modules-lib's conductor test suite
(src/conductor/__tests__/methods.test.ts) passes unchanged.
* Migrate Matrix (Tone Matrix) bundle to Conductor (#802)
* WIP: sound_matrix Conductor migration - protocol + tab (not yet building)
* WIP: bundle + tab now compile clean (fixed experimentalDecorators conflict, yarn quarantine)
* Add tests for sound_matrix bundle + tab; fix lint errors (tsc/lint/tests all clean)
* Fix hung RPC calls (missing tabLoader wiring) and tab flashing away (destroy() unregistering too eagerly)
* Fix matrix state resetting on every Run: grid must persist across Runs, not be per-instance
* Fix matrix state actually resetting across Runs: module-level state isn't real
The host loads this tab bundle via a require-wrapper (export default require
=> {...}) and calls that factory function fresh on every hostLoadPlugin, i.e.
every Run - so a plain module-level `let sharedMatrix` was reinitialised each
time despite looking module-level in source. Stash the grid on globalThis
instead, which is the one thing that's actually the same object across
repeated factory invocations in the same page.
* Fix get_matrix() row order: row 0 must be the bottom-most row
Quest Q5B's spec ("the first sound is assigned to the bottom-most row")
and the original soundToneMatrix.js (`result[i] = matrix_list[15 - i]`)
both confirm get_matrix()'s row 0 is the grid's bottom row, not the top.
The tab draws matrix[0] at the top of the canvas, so matrixToConductorList
needs to walk the array bottom-up - it was walking top-down, a real
correctness regression from the original behaviour.
* Fix CI package-info crash on a real npm dep sharing the @sourceacademy scope
topoSortPackages treated any dependency name starting with "@sourceacademy"
as a local workspace package and added it to the dependency graph. That
assumption broke the moment a bundle actually depends on the real published
@sourceacademy/conductor npm package (sound_matrix and its SoundMatrix tab
are the first to declare it directly, via "npm:^0.7.1" rather than
"workspace:^") - conductor has no RawPackageRecord since it isn't a
workspace member, so processRawPackages crashed reading .hasChanges off
undefined for it, taking down the whole "Get packages within the
repository" job (and everything downstream) on any PR touching these
packages.
Fixed by only graphing a @sourceacademy-scoped dependency when it's an
actual node in the packages record passed in, not just by name prefix.
Added a regression test mirroring this exact shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Teach the docs pipeline to document class-based bundles; fix tab's missing playwright deps
Two separate, previously-unhit CI gaps, both surfaced by sound_matrix being the
first bundle in the repo whose entire public surface is a class rather than
plain function exports:
1. modules-typedoc-plugin's json/validation code only ever understood
Function and Variable top-level reflections - a class-typed export hit the
"is a Class, which is not supported" validation warning (fatal under --ci)
with no way to produce useful docs for it either way, regardless of
whether it's a default or named export. Taught buildJson/validateModuleEntry
to flatten a Class export's own public methods (kind Method) into
individual doc entries via the same logic already used for standalone
functions - inherited members (e.g. BaseModulePlugin's `initialise`),
the constructor, and private members are excluded, so only the four
student-facing functions (get_matrix/clear_matrix/set_timeout/
clear_all_timeout) end up documented, matching what a plain-function
bundle would have produced. Added a class-shaped fixture + regression
tests covering the flattening and the exclusions.
2. SoundMatrix tab's vitest.config.ts already runs in browser mode, but its
package.json never declared the devDependencies that requires
(@vitest/browser-playwright, playwright, vitest, @vitest/coverage-v8) -
worked locally only because other tabs (Curve, Rune) happen to already
hoist them into the shared node_modules. CI's per-package scoped install
has no such luck, and separately only installs Playwright's browser
binaries when a package's own devDependencies list "playwright" - failing
with "Cannot find package 'playwright'". Added the same four
devDependencies Curve already declares for this reason.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Gemini review: matrix mutation, plugin cleanup, null canvas context
- __handleClick mutated the shared matrix array in place, then handed the
same reference back to setSharedMatrix. useSyncExternalStore's snapshot
function is getSharedMatrix itself, so React's Object.is comparison saw
no change and skipped re-rendering even though the click did register
(the canvas still drew correctly via __setColor's direct DOM write,
independent of React). Clone before mutating, matching the pattern
getMatrix() already used. Added a regression test reading the shared
matrix's global symbol directly to check the reference actually changes
on click - confirmed it fails without the fix and passes with it.
- SoundMatrixModulePlugin never cleared its pending timeouts on teardown.
A scheduled but not-yet-fired set_timeout surviving past a Run's end
would still fire later and call closure_call_unchecked against a
torn-down evaluator. Added destroy() (IPlugin's optional cleanup hook)
sharing the same clearing logic clear_all_timeout already used, now
factored into __clearAllTimeouts.
- __setColor cast getContext('2d')'s result instead of checking it -
getContext can return null, which would throw a TypeError instead of
failing gracefully.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Deep rename: sound_matrix -> matrix, no remaining references to "sound"
Per Prof Martin's request: since this bundle has no actual code-level
dependency on the sound bundle, keeping "sound_matrix"/"SoundMatrix"
naming throughout was misleading for developers - it implied a
relationship that doesn't exist. Renamed everything consistently:
- Directories: src/bundles/sound_matrix -> src/bundles/matrix,
src/tabs/SoundMatrix -> src/tabs/Matrix
- Package names: @sourceacademy/bundle-sound_matrix ->
@sourceacademy/bundle-matrix, @sourceacademy/tab-SoundMatrix ->
@sourceacademy/tab-Matrix
- Bundle id: 'sound_matrix' -> 'matrix'; typedoc name likewise
- Identifiers: SoundMatrixModulePlugin -> MatrixModulePlugin,
SoundMatrixTabPlugin -> MatrixTabPlugin, SoundMatrixTabRpc ->
MatrixTabRpc, SoundMatrixTabLoader -> MatrixTabLoader, SoundMatrixView
-> MatrixView, SO…
martin-henz
added a commit
that referenced
this pull request
Jul 31, 2026
* Add CI/CD workflow to deploy conductor modules to modules-conductor repo
Deploys build artifacts from the conductor-migration branch to
source-academy/modules-conductor via GitHub Pages, allowing the
conductor version of modules to coexist with the current deployment.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Initial Conductor Migration (Repeat Module Migration, Testing, Documentation) (#698)
* feat(repeat): migrate initial repeat module
* feat(conductor): fix conductor documentation
* chore: add testing plugin
* fix: add chaining for undefined blockTags
* fix: make changes as per review
* Apply suggestions from code review
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* Migrate binary_tree module to Conductor (#790)
* Migrate binary_tree module to Conductor
Rewrites the binary_tree bundle as a Conductor BaseModulePlugin, backed
by IDataHandler pair/list primitives instead of js-slang's stdlib. Tree
entries are stored as OPAQUE values at the module boundary; is_tree and
is_empty_tree declare no arg type so they can accept any DataType and
answer false rather than throw, matching their predicate semantics.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address review feedback: null guards and EvaluatorRuntimeError
is_tree/is_empty_tree/assertNonEmptyTree now guard against a
missing/undefined value instead of crashing with a raw TypeError, and
the plugin's arity-check throws now use EvaluatorRuntimeError (already
used elsewhere in this module) instead of a generic Error.
Also adds the same __bindExportedMethods() workaround repeat/rune use
for the unbound-method issue in BaseModulePlugin.initialise()
(conductor#41), since this fix isn't merged upstream yet.
* Validate left/right are trees in make_tree
master's binary_tree gained this validation while conductor-migration
was diverging (make_tree(0, 0, null) previously constructed a
malformed tree silently instead of throwing). Ports the same check,
using EvaluatorTypeError to match this module's existing Conductor
error style rather than modules-lib's InvalidParameterTypeError, which
doesn't apply here since this module no longer goes through js-slang.
* binary_tree: remove __bindExportedMethods workaround
The upstream binding fix (source-academy/conductor#41) is merged and
published, making the per-module bind workaround unnecessary. Also drops the
constructor override, which was left as a pure passthrough to the base class
once the workaround was removed.
* fix: pin @sourceacademy/conductor to a real published version everywhere
repeat and testplugin depended on conductor via a bare, unpinned GitHub URL.
yarn.lock had resolved and locked that to a commit from before even the
BaseModulePlugin binding fix (conductor#41) - a real `yarn install` builds
against that stale, broken conductor entirely silently, since nothing
forces Yarn to re-resolve an already-locked git dependency. Other bundles
(rune, etc.) already depend on the versioned npm release; switched these
two to match (^0.7.0), which also resolves a duplicate-package TS error
that showed up in any bundle depending on both specs simultaneously.
* binary_tree: pin @sourceacademy/conductor to a published version
Was depending on conductor via a bare, unpinned GitHub URL, which yarn.lock
had resolved and locked to a commit from before the BaseModulePlugin binding
fix (conductor#41). Switched to the versioned npm range other bundles
already use (^0.7.0), matching the same fix applied to midi.
* binary_tree: add override modifiers required by conductor-migration's stricter tsconfig
Picked up as part of merging conductor-migration in - noImplicitOverride is
now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin.
* lock file fixed
* Fix lint errors blocking pre-push: unnecessary type assertions and Conductor error allowlist
- lib/buildtools, lib/testplugin: drop assertions that no longer change
the expression's type (same pattern as master's a2f369edd fix).
- eslint.config.js: allowlist EvaluatorTypeError/EvaluatorRuntimeError in
@sourceacademy/throw-runtime-error — they're Conductor's own
protocol-level error hierarchy, unrelated to js-slang's
RuntimeSourceError that the rule checks for. binary_tree is the first
Conductor-based bundle to throw these.
- binary_tree test: drop a now-unnecessary type assertion.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: henz <henz@comp.nus.edu.sg>
* Migrate Rune Module (#765)
* feat: migrate rune
* fix: update hollusion canvas to use promise based loading
* fix: restore curve's gl-matrix
* fix: make fixes to rune and repeat
* feat: add type safety for attachModuleMethod
* feat: add custom tags
* chore: lint files
* chore: make Lee Yi's changes
* fix: post-merge fallout from conductor-migration merge
- rune bundle index.ts: add override modifiers required by the
BaseModulePlugin signature in conductor 0.7.1 (this branch was on
0.6.0 before the merge's dependency consolidation), fix implicit-any
indexing when dynamically forwarding Rune-typed static fields, and
throw GeneralRuntimeError instead of a plain Error to satisfy the
throw-runtime-error lint rule.
- Rune tab index.tsx: this file wasn't touched by the merge conflict
(only hollusion_canvas.tsx and Rune.test.tsx were), so it still
referenced the pre-rename AnaglyphRune/HollusionRune/NormalRune
classes that conductor-migration renamed to Drawn*Rune. Updated all
usages and switched the instanceof check to the isHollusionRune type
guard the bundle already exports for this purpose.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* chore: bump conductor version
* chore: add type documentation for module method attachments
* chore: add type tests
* fix: do not unregister tab on destroy
---------
Co-authored-by: Akshay-2007-1 <akshayvemulapalli2007@gmail.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: henz <henz@comp.nus.edu.sg>
* buildtools: register @publicType/@publicReturnType as known Typedoc block tags
Typedoc warned "Encountered an unknown block tag" for every @publicType/
@publicReturnType JSDoc tag since they were never registered, and CI runs
buildtools build docs with --ci (errorOnWarning), turning those warnings
into a hard failure. rune is the first bundle using these tags in real
source (repeat only exercised them in tests), so this only surfaced now.
* docs: fix 5-animations.md twoslash samples broken by rune's Conductor migration
- HollusionCanvas sample: DrawnHollusionRune.draw() is async, await it before
assigning/calling the returned render function.
- Error Handling sample: animate_rune is now a RuneModulePlugin method, not a
free export, so the import no longer type-checks as written. Widen the
twoslash directive to the errors Typescript now actually reports (2614, 7006)
instead of the stale @noErrors: 2322 from before the migration.
* Revert @functionDeclaration/@variableDeclaration decorator reimplementation
Aarav caught this: 659ea90f7 ("feat: add custom tags") deliberately removed
declarations.ts and its decorator-based type inference in favour of the
@publicType/@publicReturnType JSDoc tags normalizeSignature() already
handles. That commit never cleaned up conductor.test.ts and
fixtures/conductorDeclarations.ts, which were written earlier (cb84fefcc)
against the old decorator system - so they were orphaned, not unfinished,
when I found them failing and reimplemented the removed feature.
- utils.ts: drop the source-file/decorator parsing helpers and their use in
cloneParameter/copyPluginSignature/copyPluginVariable/isExportedVariable;
restored to match how rune's real @publicType/@publicReturnType tags are
already handled by normalisation.ts, untouched by any of this.
- conductor.test.ts: delete the test exercising the removed decorator
system; rename the rune-bundle test to reflect what it actually verifies
(@publicType/@publicReturnType tags, which pass without any of the above).
- delete the now-fully-unused fixtures/conductorDeclarations.ts.
Kept: the initTypedocForJson outDir arg fix, and the isConductorReference
qualifiedName-matching fix (traces back to 7c75a4083, predates and is
unrelated to the decorator removal).
* Migrate scrabble module to Conductor (#792)
* Migrate scrabble module to Conductor
scrabble exports four static word/letter lists rather than functions,
so there's nothing for the usual exportedNames/@moduleMethod closure
path to wrap. Exposes them via DataType.OPAQUE (opaque_make in an
initialise() override) instead of DataType.ARRAY, since array_make has
no bulk constructor and the full word list is 172,820 entries.
Also drops the two full-array snapshot tests (scrabble_words/
scrabble_letters) - snapshotting 172,820 entries produced a
multi-million-line .snap file that hung vitest's serializer. The
existing index spot-checks already cover the full arrays; only the
~1,728-entry _tiny variants are snapshotted now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Run opaque_make calls concurrently in scrabble initialise()
The four exports are independent, so awaiting them one at a time added
needless latency. Addresses gemini-code-assist review comment on #792.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Switch scrabble's word lists from OPAQUE to real DataType.ARRAY
array_make's lack of a bulk constructor (one array_set call per
element) looked prohibitive for 172,820 words, so this used
opaque_make instead. That was never measured, and the actual cost is
~246ms one-time (TestDataHandler, same-thread as the evaluator - no
postMessage boundary between a module and its evaluator) for both
scrabble_words and the nested scrabble_letters. Cheap enough that
there's no reason to give up real indexing/print/iteration for it.
Follows from source-academy/py-slang#217's module-interop fixes and
the team's decision that Python lists/JS arrays should be the only
built-in data structure modules hand back - opaque_make stays reserved
for genuinely opaque payloads (e.g. binary_tree's node values), not
plain collections.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Shrey Jain <“shreyjain5132@email.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: pin @sourceacademy/conductor to a published version everywhere (#795)
* fix: pin @sourceacademy/conductor to a real published version everywhere
repeat and testplugin depended on conductor via a bare, unpinned GitHub URL.
yarn.lock had resolved and locked that to a commit from before even the
BaseModulePlugin binding fix (conductor#41) - a real `yarn install` builds
against that stale, broken conductor entirely silently, since nothing
forces Yarn to re-resolve an already-locked git dependency. Other bundles
(rune, etc.) already depend on the versioned npm release; switched these
two to match (^0.7.0), which also resolves a duplicate-package TS error
that showed up in any bundle depending on both specs simultaneously.
* fix: address review feedback on conductor pin PR
- Add @sourceacademy/conductor to npmPreapprovedPackages to bypass the age gate
- Reset yarn.lock to base state and rerun yarn install so only conductor's resolution changes, undoing unintentional dependency downgrades
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: pin vite to a single resolved version workspace-wide
vitest's own dependency on vite ("^6.0.0 || ^7.0.0 || ^8.0.0") isn't
constrained to match the version hoisted for direct consumers
(lib/buildtools, lib/repotools), so Yarn can hoist a second, older
nested copy depending on install order. When that happens,
loadConfigFromFile (imported from the bare "vite" package) and
vitest/config's ViteUserConfig type augmentation are built against
two structurally different UserConfig types, breaking lib/repotools'
tsc with TS2339/TS2321 errors.
Reported by Prof Martin Henz while testing PR #791 locally.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: scope the vite pin from #795 to vitest only (#811)
The blanket `resolutions: { "vite": "^8.1.0" }` from #795 forced
*every* vite consumer in the workspace onto 8.1.0, including
vitepress@1.6.4 (used by the docs/docserver build), which has a
hard, non-floating dependency on vite@^5.4.14 and is not compatible
with Vite 8 internally. That broke the docserver and plotly builds
on the very next install after #795 merged.
Scope the resolution to `vitest/vite` so it only overrides vite as
resolved through vitest's own dependency graph (the actual source of
the duplicate-copy TS error), leaving vitepress's separate vite@5
tree untouched.
* fix: guard against undefined children in typedoc-plugin's buildJson (#812)
reflection.children! assumed every documented module has at least one
child reflection. That breaks for scrabble: after the conductor doc
pipeline strips its plugin class, there's nothing left to promote,
since all four of scrabble's exports (scrabble_words, etc.) are built
entirely at runtime via this.exports.push(...) rather than as
statically-analyzable class methods/properties. TypeDoc then leaves
children undefined instead of [], and the reduce() over it throws
"Cannot read properties of undefined (reading 'reduce')", which broke
build:docs (and therefore the conductor-migration -> modules-conductor
deploy) for the whole workspace as soon as #792 (scrabble) merged.
* Migrate midi module to Conductor (#791)
* Migrate midi module to Conductor
Splits midi into a pure, evaluator-free functions.ts (unchanged
signatures) and a Conductor-facing index.ts plugin, since sound and
stereo_sound import midi_note_to_frequency and friends directly as
plain TypeScript and sound/stereo_sound's own Source-facing APIs
re-export several of these functions. Migrating index.ts's exports to
require an IDataHandler would have broken both call sites immediately.
- functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic
- conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor
list conversion, accidental validation) used by the plugin; kept
separate from index.ts so they stay importable from vitest, which
hits a decorator syntax error importing index.ts directly
- index.ts: BaseModulePlugin subclass wrapping the pure functions;
SHARP/FLAT/NATURAL are pushed onto `exports` directly in the
constructor since BaseModulePlugin.initialise() only registers
exportedNames that are functions
- Includes the same __bindExportedMethods() workaround as
repeat/rune/binary_tree for the unbound-method issue in
BaseModulePlugin.initialise() (source-academy/conductor#41)
- sound/stereo_sound's functions.ts and index.ts updated to import
midi's pure functions from the new `/functions` subpath instead of
the bundle root, which now exports the Conductor plugin
* Address review feedback: use string literals instead of .name
Function.prototype.name gets mangled under minification, which would
turn these error messages into cryptic garbage like "t expects...".
Two of these were carried over unchanged from the original module; the
third is in the new Conductor-facing index.ts.
* Port midi's master-only functions and validation
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
* midi: adapt to conductor's options-object assertNumberWithinRange signature
* midi: fix scales' runtime js-slang dependency and address review feedback
scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is
unavailable under Conductor's module loader (the require() shim it's given is
a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for
any scale function. Build the intermediate list with a plain array tuple
instead, since Pair<H, T> is just [H, T] structurally.
Also addresses Aarav's review comments on #791:
- Removes __bindExportedMethods now that the upstream binding fix
(source-academy/conductor#41) is merged and published.
- Widens letter_name_to_midi_note/midi_note_to_letter_name/
letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/
get_accidental/key_signature_to_key's note/accidental parameters to plain
string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in
index.ts. Doing this exposed two real validation gaps that the casts had
been silently papering over: add_octave_to_note never validated `note` at
all (just interpolated it into the result string), and
midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP
accidental as FLAT instead of rejecting it. Both now throw
EvaluatorParameterTypeError for invalid input, with regression tests added.
* midi: drop unnecessary js-slang runtime dependency, fix real dependency resolution
scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type
shape, not js-slang itself. Replaced with a local Scale type; js-slang moves
to devDependencies since only the test suite still uses it.
Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub
URL, which yarn.lock had resolved and locked to a commit from before even the
BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using
a local portal: link for testing) was building against that stale, broken
conductor entirely silently. Switched to the same versioned npm range other
bundles already use (^0.7.0), and reverted the two calls to
assertNumberWithinRange that had been written against conductor PR #43's
still-unpublished options-object signature back to the options actually
published in 0.7.0.
* midi: add override modifiers and fix scale test type error
Picked up as part of merging conductor-migration in - noImplicitOverride
is now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin. Also fixed a test that built its input
scale via js-slang's untyped list() instead of midi's own js-slang-free
Scale shape, which only surfaced as a real tsc error once the merge
brought stricter checking back online.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: align @sourceacademy/conductor pin with catalog: convention
Matches the same fix already applied on #795 and #796: adds
@sourceacademy/conductor to the yarn catalog and
npmPreapprovedPackages, and switches midi's own package.json to
"catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't
leave midi on the old convention once #795 lands.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: reuse parseNoteWithOctave as the canonical validator in add_octave_to_note
Addresses CodeRabbit's review comment on PR #796 (surfaced there via
shared branch history with sound, but the finding is midi's own):
add_octave_to_note's inline regex accepted note spellings that
noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb -
accidentals that don't exist for those note names), and let lowercase
input escape unnormalized through a type assertion. Now validates via
parseNoteWithOctave (rejecting digits upfront, since that function
alone would accept an octave already being present) and reconstructs
using the normalized note name, preserving the original accidental
spelling exactly as given.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Pin vite to one version workspace-wide, fixing a local yarn install failure
Prof Martin hit a build failure running yarn install locally: lib/repotools'
own tsc failed with "Property 'test' does not exist on type 'UserConfig'"
and "Excessive stack depth comparing types 'UserConfig' and 'UserConfig'"
in src/testing/index.ts.
Root cause: two different resolved copies of vite existed side by side -
the workspace root hoists vite@8.1.4 (requested directly by lib/buildtools
and devserver, both "^8.1.0"), but vitest@4.1.9's own internal dependency
on vite resolved its own nested copy at 8.0.12 instead, since nothing
constrained it to match. src/testing/index.ts imports loadConfigFromFile
from the bare "vite" package (resolving to the hoisted 8.1.4) while
vitest/config's ViteUserConfig type augmentation is built against vitest's
own nested 8.0.12 UserConfig - TypeScript sees these as two structurally
different types instead of the same one, hence the errors. Reported as
non-deterministic since it depends on exactly how Yarn happens to hoist
things on a given install.
Fixed with a resolutions entry pinning every resolution of vite to
^8.1.0 workspace-wide (matching Prof Martin's own suggested fix) - the
nested copy under vitest/node_modules is now gone entirely after
reinstalling, both consumers resolve the identical module, and
lib/repotools' tsc (and lib/buildtools', the other direct vite consumer)
are both clean.
* Add real JSDoc to index.ts's wrapper methods so docs actually regenerate
Mirrors rune's pattern (#765): the doc generator reads JSDoc off the
@moduleMethod-decorated wrapper methods in index.ts, not off the real
implementations in functions.ts/scales.ts they delegate to - a thin
wrapper with no comment of its own means the generated docs.json comes
back "No description available" for every export, even though the real
JSDoc is sitting right there on the underlying function.
Copied each wrapper's description/@param/@returns/@example from its
functions.ts/scales.ts counterpart. No @publicType/@publicReturnType
overrides needed here (unlike rune's Rune/OPAQUE case) - every one of
midi's parameter and return types (NUMBER, CONST_STRING, BOOLEAN, LIST)
already has an unambiguous native mapping.
Verified by rebuilding docs: all 19 exports now show real descriptions
instead of "No description available" (confirmed per-export, not just
absence of warnings). tsc/lint/test (33/33) all clean.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: regenerate yarn.lock, drifted out of sync after midi (#791) merge
yarn install --immutable was failing on conductor-migration ("The lockfile
would have been modified by this install") since the midi merge, breaking
the deploy-to-modules-conductor pipeline at the Install Dependencies step.
The committed lockfile was missing a batch of esbuild@0.21.5 optional
platform-dependency entries that a fresh install adds back. Regenerated
via a plain yarn install; no dependency version changes, just filling in
the entries the merge left out.
* Migrate Sound Module to Conductor (#796)
* Migrate midi module to Conductor
Splits midi into a pure, evaluator-free functions.ts (unchanged
signatures) and a Conductor-facing index.ts plugin, since sound and
stereo_sound import midi_note_to_frequency and friends directly as
plain TypeScript and sound/stereo_sound's own Source-facing APIs
re-export several of these functions. Migrating index.ts's exports to
require an IDataHandler would have broken both call sites immediately.
- functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic
- conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor
list conversion, accidental validation) used by the plugin; kept
separate from index.ts so they stay importable from vitest, which
hits a decorator syntax error importing index.ts directly
- index.ts: BaseModulePlugin subclass wrapping the pure functions;
SHARP/FLAT/NATURAL are pushed onto `exports` directly in the
constructor since BaseModulePlugin.initialise() only registers
exportedNames that are functions
- Includes the same __bindExportedMethods() workaround as
repeat/rune/binary_tree for the unbound-method issue in
BaseModulePlugin.initialise() (source-academy/conductor#41)
- sound/stereo_sound's functions.ts and index.ts updated to import
midi's pure functions from the new `/functions` subpath instead of
the bundle root, which now exports the Conductor plugin
* Address review feedback: use string literals instead of .name
Function.prototype.name gets mangled under minification, which would
turn these error messages into cryptic garbage like "t expects...".
Two of these were carried over unchanged from the original module; the
third is in the new Conductor-facing index.ts.
* Port midi's master-only functions and validation
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
* midi: adapt to conductor's options-object assertNumberWithinRange signature
* midi: fix scales' runtime js-slang dependency and address review feedback
scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is
unavailable under Conductor's module loader (the require() shim it's given is
a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for
any scale function. Build the intermediate list with a plain array tuple
instead, since Pair<H, T> is just [H, T] structurally.
Also addresses Aarav's review comments on #791:
- Removes __bindExportedMethods now that the upstream binding fix
(source-academy/conductor#41) is merged and published.
- Widens letter_name_to_midi_note/midi_note_to_letter_name/
letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/
get_accidental/key_signature_to_key's note/accidental parameters to plain
string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in
index.ts. Doing this exposed two real validation gaps that the casts had
been silently papering over: add_octave_to_note never validated `note` at
all (just interpolated it into the result string), and
midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP
accidental as FLAT instead of rejecting it. Both now throw
EvaluatorParameterTypeError for invalid input, with regression tests added.
* midi: drop unnecessary js-slang runtime dependency, fix real dependency resolution
scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type
shape, not js-slang itself. Replaced with a local Scale type; js-slang moves
to devDependencies since only the test suite still uses it.
Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub
URL, which yarn.lock had resolved and locked to a commit from before even the
BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using
a local portal: link for testing) was building against that stale, broken
conductor entirely silently. Switched to the same versioned npm range other
bundles already use (^0.7.0), and reverted the two calls to
assertNumberWithinRange that had been written against conductor PR #43's
still-unpublished options-object signature back to the options actually
published in 0.7.0.
* midi: add override modifiers and fix scale test type error
Picked up as part of merging conductor-migration in - noImplicitOverride
is now enabled repo-wide, and exportedNames/channelAttach shadow members
declared on BaseModulePlugin. Also fixed a test that built its input
scale via js-slang's untyped list() instead of midi's own js-slang-free
Scale shape, which only surfaced as a real tsc error once the merge
brought stricter checking back online.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: rebuild playback/recording on a self-contained Conductor channel
Scraps the earlier plugins-repo sound-io plugin pair in favor of the
pattern confirmed against rune's migration (PR #765): SoundModulePlugin
declares its own channelAttach directly, and modules/src/tabs/Sound
implements IPlugin/SoundTabRpc as the host-side counterpart, talking
over Conductor's makeRpc helper - no separate runner/web plugin
package needed at all.
Also fixes plotly's draw_sound_2d, the one other consumer of
bundle-sound's types, for the Wave type's redesign from a plain
(t: number) => number to an async generator (so that stepping and
nested user closures evaluate correctly on the CSE machine).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: unify with stereo_sound - one module, Sound is always stereo
Retires stereo_sound as a separate bundle/tab entirely. Sound becomes
{leftWave, rightWave, duration} always; "mono" isn't a separate type,
it's just the common case where leftWave === rightWave (same
reference), produced by make_sound. make_stereo_sound builds a
genuinely stereo Sound from two different waves. Every combinator
(consecutively/simultaneously/adsr/phase_mod/stacking_adsr/
instruments) now operates on this one shape via small per-channel
helpers (joinWaves/sumWaves/adsrWave/phaseModWave), so composing a
"mono" sound with a stereo one is a non-issue - there's nothing to
convert, and none of stereo_sound's oscillator/envelope math is
duplicated a second time the way it was as a separate bundle.
get_wave/get_left_wave/get_right_wave: get_wave keeps meaning "the"
wave (== left channel), so existing SICP-style code (play(sine_sound(...)),
get_wave(sound)) keeps working unchanged for the common mono case.
Adds a lower Wave-returning layer alongside the existing Sound layer
(sine_wave/square_wave/triangle_wave/sawtooth_wave/noise_wave/
silence_wave), with sine_sound etc. as thin convenience wrappers
(sine_sound = (freq, dur) => make_sound(sine_wave(freq), dur)).
record()/record_for() use however many channels the input device
actually has - a mono microphone (the common case) produces a Sound
whose left and right channels are the same wave; no separate
record_stereo. play/samplesToSound skip redoing work for a mono
Sound (sampled once, not twice) by checking leftWave === rightWave.
New stereo-specific operations: make_stereo_sound, play_waves,
pan, pan_mod, squash, get_left_wave, get_right_wave.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: rebuild Sound/functions/index/tab for the sound+stereo_sound unification
Continuation of the previous commit (which only picked up the
stereo_sound/StereoSound deletions due to a failed multi-pathspec git
add) - this is the actual Sound={leftWave,rightWave,duration} rework
of the sound bundle and its tab described there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* align @sourceacademy/conductor pins with PR #795's catalog: convention
#795 (fix/conductor-dependency-pin) is about to merge into
conductor-migration first, establishing @sourceacademy/conductor in
the yarn catalog and npmPreapprovedPackages. Switching midi/sound/
tab-Sound's package.json over to "catalog:" now (matching #795
verbatim) avoids re-doing this exact reconciliation the next time
this branch merges conductor-migration in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: fix O(N) nested yield* delegation in consecutively/simultaneously
Addresses Gemini's high-priority review comments on PR #796: reduce()
was nesting joinWaves/sumWaves closures sounds.length deep, so
sampling near the end of a long chain meant up to N nested async
generator delegations per sample at 44100 Hz - real overhead for
async generators that plain synchronous functions (the pre-migration
implementation) never had. Rewritten as a flat scan that picks the
active sound directly and yield*s into it once, keeping delegation
depth O(1) regardless of how many sounds are combined, while still
threading through user closures correctly.
Also fixes a real (if narrow) NaN: linear_decay(0) computed 1 - 0/0
when release_ratio (or a future caller with decay_ratio) is exactly
0, corrupting the sample at that instant. Sample rate/instrument
functions never happened to hit it because of how the surrounding
branch conditions are structured, but adsr's release branch can.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: align @sourceacademy/conductor pin with catalog: convention
Matches the same fix already applied on #795 and #796: adds
@sourceacademy/conductor to the yarn catalog and
npmPreapprovedPackages, and switches midi's own package.json to
"catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't
leave midi on the old convention once #795 lands.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: address CodeRabbit review findings on PR #796
- validateDuration now rejects NaN/Infinity, not just negatives
(both used to slip through and crash sampleWave's Float32Array
allocation with a raw RangeError instead of a clean module error).
- closureToWave validates the closure's result is actually a number
before returning it - a student-supplied wave returning e.g. a
string previously corrupted playback silently via `as number`.
- stacking_adsr validates each envelope-list element is a closure
before invoking it, for the same reason.
- consecutively/simultaneously/adsr/phase_mod/conductorToSound all
preserve the "mono means leftWave === rightWave" invariant again -
building both channels independently (even from identical inputs)
produced two behaviourally-identical-but-distinct waves, silently
doubling sampling work and losing the fast path in play()/etc.
- play()/stop() guard against a stale playSamples() completion
clobbering a newer play()'s isPlaying state via a generation token
(stop-A/start-B/late-settle-A ordering).
- tab: requestMicPermission() disposes the previous MediaStream
before re-requesting, so a denied re-request can't leave stale
tracks running and reusable by startRecording().
- tab: startRecording() now actually awaits MediaRecorder's start
event (and rejects on error) instead of resolving as soon as
.start() returns, matching the SoundTabRpc contract.
Not addressed (flagged as false positive or out of scope for a
quick fix, not silently dropped):
- The module doc's "a wave returns a number" description is correct
as written - it's the student-facing Source-language contract, not
the internal async-generator implementation detail (which is
already documented separately on the Wave type in types.ts).
- Overlapping/concurrent recording sessions (record/record_for only
gate on isPlaying, not a dedicated recording-state flag) and
pan/pan_mod sampling the shared source/modulator wave twice per
channel are both real but non-trivial fixes requiring more
substantial state-tracking/sampling-order changes; left for a
follow-up rather than a rushed partial fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* sound: cross-reference Wave's internal async-generator contract in the module doc
Partial concession on CodeRabbit's doc-comment finding: the primary
description stays as the Source-facing "number -> number" contract
(that's the actual, correct student experience - the CSE machine
threads the async-generator machinery transparently), but adds a
one-line pointer to where the internal TS contract is documented, for
maintainers reading this file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* midi: reuse parseNoteWithOctave as the canonical validator in add_octave_to_note
Addresses CodeRabbit's review comment on PR #796 (surfaced there via
shared branch history with sound, but the finding is midi's own):
add_octave_to_note's inline regex accepted note spellings that
noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb -
accidentals that don't exist for those note names), and let lowercase
input escape unnormalized through a type assertion. Now validates via
parseNoteWithOctave (rejecting digits upfront, since that function
alone would accept an octave already being present) and reconstructs
using the normalized note name, preserving the original accidental
spelling exactly as given.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix playback/recording races and add ADSR validation, found via manual testing
Manually testing all three migrated modules (binary_tree, midi, sound)
together against a local frontend build surfaced several issues
specific to sound, now fixed:
- play() threw "Previous sound still playing!" on any overlapping or
looped call. Repeated/looped play() calls now queue and play one
after another instead (like consecutively, but built up call-by-call
rather than pre-combined into one Sound). stop() cancels anything
still queued behind the currently-playing sound, not just the
current one.
- The tab's "Constructing..." status (shown while play() samples a
Wave into a buffer - a duration-proportional step that happens
entirely before actual playback starts) never actually appeared,
since the notification was fire-and-forget and could race the tab's
own asynchronous loading. Now a real acknowledged RPC call, awaited
before sampling begins.
- init_record() was fire-and-forget: it kicked off the permission
request but returned immediately, so calling record()/record_for()
right after (the natural way to write it) could race the still-
pending permission grant and throw even though permission had
genuinely just been granted. init_record() now awaits the actual
permission result before returning.
- record()/record_for()'s returned "sound promise" threw "recording
still being processed" until called again later, requiring manual
polling. Both now return a promise that genuinely awaits the
recording finishing processing instead.
- adsr() had no validation that attack_ratio + decay_ratio +
release_ratio stays within 1, or that each ratio/sustain_level is a
finite number in [0, 1] - silently producing a discontinuous
envelope instead of an error. Added validation, with the actual
envelope-shaping logic split into an unvalidated adsrTransformer()
used internally by piano/violin/cello/trombone/bell, so validating
adsr() doesn't retroactively break trombone's second-harmonic
envelope (ratios summing to 1.0236, a quirk present since before
the Conductor migration - preserved rather than silently changed).
* Move playback sequencing to the host tab; fix premature AudioContext teardown
Found via extensive live testing against a local frontend build:
- The Worker running a program is terminated as soon as the script
finishes (conduit.terminate(), called after every Run to fix a
worker-leak). play() is intentionally fire-and-forget, so a script
can finish - and the Worker be killed - well before queued playback
has actually started or finished. Sequencing that playback via a
Worker-side queue (the previous design) meant anything still queued
when the Worker died simply never got its playSamples() RPC sent at
all.
- functions.ts' play() now dispatches its playSamples() call
immediately once sampling finishes, instead of waiting its turn in
a local queue - so the RPC always gets sent before the Worker can be
torn down. Actual sequencing (so playback doesn't overlap) moves to
SoundTabPlugin on the host side, which outlives the Worker: it now
owns its own playback queue and a stop-generation counter so a
still-queued call correctly gets cancelled by stop().
- SoundTabPlugin.destroy() (called on every Run's teardown) no longer
closes the AudioContext or unregisters the tab immediately - both
are deferred until whatever's playing (or still queued) finishes
naturally, tracked via a dedicated pending-playback counter rather
than activeSources.size, which hits 0 momentarily between any one
sound ending and the next queued one starting and was otherwise
misread as "everything is done," silently killing the AudioContext
mid-queue.
- notifyConstructing()/playSamples() status updates are now
recomputed from combined constructing+active-source state instead
of set unconditionally, so an earlier sound finishing can't clobber
a later sound's still-in-flight 'constructing' status.
* sound: fast synchronous path for module-native waves, skip per-sample async overhead
Wave is AsyncGenerator-based so a wave wrapping a user-supplied Conductor closure
(closureToWave, in index.ts) can be driven through evaluator.closure_call_unchecked -
itself an AsyncGenerator, since it steps the calling evaluator's CSE machine. But
every wave built entirely from module-native math (oscillators, envelopes,
instruments, and every combinator) got the same generator wrapper even though it
never actually yields - and every AsyncGenerator#next() resolves via microtask
regardless of whether anything inside really awaits. sampleWave calls a wave once
per sample (44100 times per second of audio), so a multi-second Sound made of nothing
but built-in instruments was paying real async overhead - measured as the dominant
cost of play() for a ~21s cello passage - for zero actual asynchrony.
Adds Wave.sync: an optional plain (t: number) => number twin, present iff a wave
provably never needs to cross into user code. syncWave() builds both forms from one
computation. Every combinator (clipToDuration, consecutiveWave, simultaneousWave,
adsrWave, phaseModWave, gainWave, squash, panModAmountWave, pan_mod, interpolatedWave)
propagates sync when every wave feeding into it has one, and falls back to the
existing yield*-driven path the moment a closure-backed wave (which never sets sync)
is anywhere in the composition - so a student-supplied wave function's stepping/
breakpoint visibility, and Conductor's worker-boundary async contract, are entirely
unaffected. sampleWave itself takes the fast path whenever the wave it's sampling has
one.
Confirms the fast path is a pure performance change, not a semantic one: correctness
depends only on the fallback being exact, which the existing PAIR/ARRAY module-interop
and existing sound-bundle test suite (64 tests, all passing) already exercise the
composition shapes for.
* sound: sync fast path for student-authored wave closures, when the evaluator supports it
closureToWave wrapped every student-supplied wave function as a plain async
generator calling evaluator.closure_call_unchecked - correct, but paying a real
microtask (and a full evaluator round-trip) per sample even for the simplest
possible wave, sampled 44100x/sec. The built-in-wave fast path landed earlier in
this file's siblings (functions.ts) doesn't help here: a student's own wave has
to actually run inside whichever engine (CSE/PVML/py2js) is executing the
program - there's no way for the module itself to skip that call.
An engine can now opt a closure into a synchronous fast path by implementing
closure_call_sync (py-slang's GenericDataHandler, exposed so far only by py2js,
whose dual-compiled functions already have a synchronous body internally -
rt.callSync - that just wasn't reachable from outside py-slang before). This
module checks for the method generically - closureToWave attaches wave.sync
only when the evaluator actually provides closure_call_sync, and every other
engine (CSE, PVML, until they grow the same capability) is completely
unaffected: the method simply doesn't exist on their evaluator instance, so
every wave stays on the existing async path, unchanged.
The one correctness wrinkle worth being explicit about: closure_call_sync
returning undefined means "no sync form for this closure" - safe as a signal
before the closure has run, but from inside wave.sync (a plain function, not a
generator - there's no way to suspend and retry via the async path once we're
in here) a missing sync form after the fact is treated as a hard internal
error rather than silently producing a wrong sample.
* sound: waveToConductorClosure was silently dropping the sync fast path
closureToWave (Conductor closure -> internal Wave) already picks up wave.sync
from the evaluator's closure_call_sync when available, but the reverse
direction didn't: waveToConductorClosure (internal Wave -> Conductor closure)
always built a plain async-only wrapper, with no way for anything downstream
to know the original wave ever had a sync form. Since make_sound/
make_stereo_sound/get_wave/etc. all round-trip a Sound's waves through this
function to hand it back to Python, every Sound handed back to a student -
even one built entirely from a py2js closure that supports closure_call_sync -
permanently lost the fast path the instant it was constructed. play() on that
same Sound later would then find closure_call_sync returning undefined (no
.sync on the re-wrapped closure) and hit the "no synchronous form" internal
error closureToWave raises for exactly this shouldn't-happen case.
Fixed by having waveToConductorClosure attach the same kind of .sync twin
(reusing wave.sync, converting its number result to/from a TypedValue) before
handing the function to closure_make, mirroring closureToWave's direction
exactly. No behavior change for a wave with no sync form (CSE closures, or a
wave that genuinely needs a host round-trip) - conductorWave.sync is simply
never attached.
* sound: address review feedback on record/record_for and error messages
- record/record_for: stop hardcoding the function name in error messages
(record: ..., record_for: ...) - use `${record.name}`/`${record_for.name}`
like every other error in this file, so a rename can't silently go stale.
Same fix for play's EvaluatorParameterTypeError/duration-negative error,
which had the same hardcoded-literal bug.
- record/record_for: replace the nested setTimeout+.then() towers with a
single async IIFE using es-toolkit's delay(), preserving the exact same
event ordering (pre-recording-signal pause, recording signal, pre-recording
pause, recording, recording signal) but as flat sequential awaits.
* sound: accept DataType.ARRAY as equally valid to PAIR for Sound values
py-slang's module interface no longer has a distinct "pair" representation
(source-academy/py-slang#307): pythonToModule now builds every Python list/
pair as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A Sound
built here via soundToConductor and round-tripped out to Python (assigned to
a variable, passed to another module call like get_wave/is_sound/play) then
arrives tagged ARRAY, not PAIR - conductorToSound/is_sound hardcoded
`value.type !== DataType.PAIR` checks broke on this, same class of bug as
binary_tree's #813.
Fixed the same way: isPairLike(value) accepts either DataType.PAIR or
DataType.ARRAY wherever a Sound's own tag is checked - pair_head/pair_tail
already read either shape identically (py-slang's GenericDataHandler
bridge), so this is purely about validation, not traversal.
Also fixed conductorListToSounds (consecutively/simultaneously's list
argument) and stacking_adsr's envelopes list: a genuine Python list of any
length now crosses as a flat ARRAY too, not just a 2-element pair, so the
old PAIR-chain-only walk would silently return zero elements for a real
multi-sound list. Added readListElements to read either an ARRAY (via
array_length/array_get) or a PAIR/EMPTY_LIST chain, matching py-slang's own
GenericDataHandler.readListElements pattern.
soundToConductor's own construction (pair_make calls) is unchanged - it
always freshly builds a genuine PAIR; only validation of an incoming
(possibly round-tripped) value needed to widen.
* sound: don't crash when closure_call_sync exists but this closure has no .sync twin
closure_call_sync lives on GenericDataHandler, the shared IDataHandler
implementation across all of py-slang's engines - it's always present
regardless of which engine is actually running, so closureToWave's old check
("does the method exist") was never actually engine-specific despite its own
comment claiming otherwise. Only some py2js closures ever carry a real
.sync twin; CSE and PVML closures never do. Every student-authored wave
function played via play()/play_wave() on CSE or PVML was hitting the
'Internal error: closure_call_sync unexpectedly had no synchronous form'
throw on its very first sample, since .sync was attached unconditionally
whenever the method merely existed.
Fixed by determining sync-capability once, with a real probe call, before
ever exposing .sync on the Wave at all - closure_call_sync's own contract
only returns undefined for an unsupported argument type before the closure
ever runs (a wave's argument is always a plain number, always supported),
so the probe is free unless the closure genuinely has a .sync twin, in
which case its result is reused as the Wave's first real sample instead of
being thrown away. If .sync isn't available, the Wave silently stays on the
existing async path - no crash, matches every other Sound consumer's
existing async fallback.
* sound: correct closure-cache comment per Martin's ruling on identity preservation
Martin: the module-evaluator bridge isn't obligated to be identity-preserving
- two JS functions that are === may be represented by two Python functions
that aren't 'is' to each other, and that's fine as a general FFI property.
The earlier comment claimed the wave/closure caches make
get_wave(s) == get_left_wave(s) a reliable invariant - they don't, since
each engine's own moduleToPython still builds a fresh Python-side wrapper
per conversion regardless of what's cached on the Conductor side. The
caches stay (still real, just for avoiding redundant closure_make calls),
the comment now says what they actually guarantee.
* sound: describe mono behaviorally, not by reference identity, in doc comments
Per Martin: avoid 'identical' when describing the two channels of a mono
Sound in specs - say left_wave(t) == right_wave(t) for all t instead of
claiming the two waves are the same object/reference. Reworded the public-
facing docblocks (index.ts's and functions.ts's @module comments, the Sound
type's own doc in types.ts, get_wave/record's JSDoc) to describe the
behavioral contract a cadet program can actually observe, rather than an
internal reference-equality detail. Where a comment is genuinely about this
file's own implementation choice (make_sound assigning one Wave object to
both fields, and later code taking advantage of that via leftWave ===
rightWave checks), left those as-is and called out explicitly that it's an
implementation detail, not something the Sound Discipline promises.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* sound: fix lint errors on conductor-migration (#818)
* sound: fix lint errors - EvaluatorParameterTypeError gap and a plain Error
@sourceacademy/throw-runtime-error doesn't yet recognize
EvaluatorParameterTypeError as assignable to RuntimeSourceError (same known
gap already worked around in midi) - added the same eslint-disable +
explanatory comment at each of the 6 call sites in functions.ts/index.ts.
conductorToSound's invalid() helper was throwing a plain Error, not a
Conductor error type at all - a real bug, not just a lint gap, since it
would bypass proper student-facing error formatting. Fixed to
EvaluatorRuntimeError; inlined the three call sites since the indirection
through a helper function was itself what kept the linter from recognizing
a type it accepts fine when thrown directly.
The constructor's soundChannel wiring guard is left as a plain Error with a
disable comment - a genuine internal precondition (Conductor host failed to
provide the channel), never reachable from student code.
Also fixed two @stylistic/member-delimiter-style warnings in
recording.test.ts (semicolons vs commas in an inline type).
* docs: fix stale sound-module type references breaking the docs build
#796's Conductor migration removed AudioPlayed/SoundModuleState entirely
(the old js-slang moduleContexts.sound.state pattern doesn't apply to
Conductor-based modules) and reshaped Sound from a Pair to
{leftWave, rightWave, duration} - several doc pages' embedded, type-checked
code samples still referenced the old shapes, failing the docs build's
twoslash validation:
- 2-bundle/4-conventions/3-errors.md: make_sound sample used pair() from
js-slang/dist/stdlib/list against the old Sound type - rewritten to
return the real {leftWave, rightWave, duration} shape.
- 5-advanced/context.md, 3-tabs/1-overview.md, 3-tabs/3-editing.md: all
three used sound as their example bundle for the general js-slang module
context / getModuleState pattern, referencing AudioPlayed/SoundModuleState
which no longer exist at all. Swapped to curve, which still legitimately
uses this pattern (sound moved to Conductor's own channel-based state
instead) - reused curve's actual CurveModuleState/drawnCurves shape and
the real Curve tab's own getModuleState usage as the reference.
Verified the two rewritten make_sound/context samples against a real tsc
--strict run (not just eyeballing) - both type-check clean.
4-testing/4-unit/3-mock.md and 2-bundle/1-overview/1-overview.md also
reference bundle-sound but only via imports/calls that stay valid regardless
of Sound's internal shape (no destructuring assuming the old pair shape) -
left as-is, not build-blocking, though mock.md's AudioContext example is now
semantically stale (play() no longer touches AudioContext directly) and
could use a follow-up pass.
* docs: fix CurveDrawn not being an exported type from bundle-curve/types (#819)
CurveDrawn is imported into curve/types.ts from ./curves_webgl but never
re-exported from it - only CurveModuleState (which uses CurveDrawn as part
of its drawnCurves field type) is actually exported from
@sourceacademy/bundle-curve/types. #818's context.md fix imported CurveDrawn
directly, which doesn't exist at that import path (TS2459), breaking the
docs build again. Fixed by deriving the element type via
CurveModuleState['drawnCurves'] instead - the same pattern already used
correctly in this PR's other two fixed files (3-tabs/1-overview.md,
3-tabs/3-editing.md), just missed here.
Verified against a real tsc --strict run this time, not just eyeballing -
all imports across all 4 previously-touched doc pages checked against their
actual compiled .d.ts exports, not just what's declared in bundle source.
* Fix predicate methods declaring 0 arity instead of DataType.ANY (#821)
is_sound (sound), is_tree/is_empty_tree (binary_tree), and
is_note_with_octave (midi) are @moduleMethod-decorated predicates meant to
accept one value of any Conductor DataType and answer false rather than
throw. They were declared with an empty args array ([]), which correctly
signals "no fixed type" but incorrectly reports arity 0 via
closure_arity() - the array's length is the only thing engines can read
back as "how many parameters this closure takes."
Every engine reads this same signature (GenericDataHandler.closure_arity),
but only py2js treats it as an exact contract, so a real call like
is_sound(s) threw "is_sound() takes 0 arguments but 1 was given" there
while CSE and PVML worked fine. Found while debugging that report.
DataType.ANY exists in conductor precisely for "one argument, unrestricted
type" - using it instead of [] reports the correct arity (1) without
changing any runtime behavior (GenericDataHandler never type-checks args
against the declared signature, only counts them). Requires bumping the
conductor catalog range to ^0.7.2 (ANY was added in 0.7.1; 0.7.0 predates
it) and normalizing binary_tree's package.json off a hardcoded
"^0.7.0" onto the shared "catalog:" entry, which was silently out of step
with every other bundle.
Claude-Session: https://claude.ai/code/session_01KrwJGug3rCXijRseRjys9V
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Bump @sourceacademy/conductor catalog pin to 0.8.0 (#822)
PR source-academy/conductor#53 makes hostLoadPlugin/requestLoadPlugin
async (Promise<void> instead of void) and renames the RPC procedure
from the notification $requestLoadPlugin to the awaitable
requestLoadPlugin.
No module in this repo calls hostLoadPlugin/requestLoadPlugin directly
(only IModulePlugin/type-level conductor surface is used), so this is
purely a version bump. modules-lib's conductor test suite
(src/conductor/__tests__/methods.test.ts) passes unchanged.
* Migrate pix_n_flix module to Conductor
Replaces the array-of-arrays Pixel/Pixels/Filter API with opaque image
handles accessed via get_pixel_value/set_pixel_value, matching the
opaque-handle plus accessor-function design Martin Henz laid out for
this module. Splits the bundle/tab boundary into a control channel
(makeRpc, dimensions/fps/volume/input source) and a dedicated frame
channel that transfers raw ArrayBuffers instead of structured-cloning
them, since a full video frame would otherwise get copied twice per
frame. get_pixel_value/set_pixel_value carry a sync fast path backed by
the module's own buffer registry, so per-pixel access from a student
filter avoids the async-generator round trip on every call.
All camera/video/canvas/requestAnimationFrame ownership moves to the
tab, since the module now runs in a Worker with no DOM access.
Depends on two engine-side PRs (conductor#54, py-slang#353) for the
sync fast path to actually take effect at runtime; falls back to the
existing async path until those land.
* Migrate Matrix (Tone Matrix) bundle to Conductor (#802)
* WIP: sound_matrix Conductor migration - protocol + tab (not yet building)
* WIP: bundle + tab now compile clean (fixed experimentalDecorators conflict, yarn quarantine)
* Add tests for sound_matrix bundle + tab; fix lint errors (tsc/lint/tests all clean)
* Fix hung RPC calls (missing tabLoader wiring) and tab flashing away (destroy() unregistering too eagerly)
* Fix matrix state resetting on every Run: grid must persist across Runs, not be per-instance
* Fix matrix state actually resetting across Runs: module-level state isn't real
The host loads this tab bundle via a require-wrapper (export default require
=> {...}) and calls that factory function fresh on every hostLoadPlugin, i.e.
every Run - so a plain module-level `let sharedMatrix` was reinitialised each
time despite looking module-level in source. Stash the grid on globalThis
instead, which is the one thing that's actually the same object across
repeated factory invocations in the same page.
* Fix get_matrix() row order: row 0 must be the bottom-most row
Quest Q5B's spec ("the first sound is assigned to the bottom-most row")
and the original soundToneMatrix.js (`result[i] = matrix_list[15 - i]`)
both confirm get_matrix()'s row 0 is the grid's bottom row, not the top.
The tab draws matrix[0] at the top of the canvas, so matrixToConductorList
needs to walk the array bottom-up - it was walking top-down, a real
correctness regression from the original behaviour.
* Fix CI package-info crash on a real npm dep sharing the @sourceacademy scope
topoSortPackages treated any dependency name starting with "@sourceacademy"
as a local workspace package and added it to the dependency graph. That
assumption broke the moment a bundle actually depends on the real published
@sourceacademy/conductor npm package (sound_matrix and its SoundMatrix tab
are the first to declare it directly, via "npm:^0.7.1" rather than
"workspace:^") - conductor has no RawPackageRecord since it isn't a
workspace member, so processRawPackages crashed reading .hasChanges off
undefined for it, taking down the whole "Get packages within the
repository" job (and everything downstream) on any PR touching these
packages.
Fixed by only graphing a @sourceacademy-scoped dependency when it's an
actual node in the packages record passed in, not just by name prefix.
Added a regression test mirroring this exact shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Teach the docs pipeline to document class-based bundles; fix tab's missing playwright deps
Two separate, previously-unhit CI gaps, both surfaced by sound_matrix being the
first bundle in the repo whose entire public surface is a class rather than
plain function exports:
1. modules-typedoc-plugin's json/validation code only ever understood
Function and Variable top-level reflections - a class-typed export hit the
"is a Class, which is not supported" validation warning (fatal under --ci)
with no way to produce useful docs for it either way, regardless of
whether it's a default or named export. Taught buildJson/validateModuleEntry
to flatten a Class export's own public methods (kind Method) into
individual doc entries via the same logic already used for standalone
functions - inherited members (e.g. BaseModulePlugin's `initialise`),
the constructor, and private members are excluded, so only the four
student-facing functions (get_matrix/clear_matrix/set_timeout/
clear_all_timeout) end up documented, matching what a plain-function
bundle would have produced. Added a class-shaped fixture + regression
tests covering the flattening and the exclusions.
2. SoundMatrix tab's vitest.config.ts already runs in browser mode, but its
package.json never declared the devDependencies that requires
(@vitest/browser-playwright, playwright, vitest, @vitest/coverage-v8) -
worked locally only because other tabs (Curve, Rune) happen to already
hoist them into the shared node_modules. CI's per-package scoped install
has no such luck, and separately only installs Playwright's browser
binaries when a package's own devDependencies list "playwright" - failing
with "Cannot find package 'playwright'". Added the same four
devDependencies Curve already declares for this reason.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Gemini review: matrix mutation, plugin cleanup, null canvas context
- __handleClick mutated the shared matrix array in place, then handed the
same reference back to setSharedMatrix. useSyncExternalStore's snapshot
function is getSharedMatrix itself, so React's Object.is comparison saw
no change and skipped re-rendering even though the click did register
(the canvas still drew correctly via __setColor's direct DOM write,
independent of React). Clone before mutating, matching the pattern
getMatrix() already used. Added a regression test reading the shared
matrix's global symbol directly to check the reference actually changes
on click - confirmed it fails without the fix and passes with it.
- SoundMatrixModulePlugin never cleared its pending timeouts on teardown.
A scheduled but not-yet-fired set_timeout surviving past a Run's end
would still fire later and call closure_call_unchecked against a
torn-down evaluator. Added destroy() (IPlugin's optional cleanup hook)
sharing the same clearing logic clear_all_timeout already used, now
factored into __clearAllTimeouts.
- __setColor cast getContext('2d')'s result instead of checking it -
getContex…
This was referenced Aug 2, 2026
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
3 participants
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
soundbundle to the Conductor framework, and unifies it withstereo_sound(retired entirely - no separate bundle/tab): Sound is now always{leftWave, rightWave, duration}internally. "Mono" isn't a separate type, it's just the common case whereleftWave === rightWave(same reference), produced bymake_sound. Every combinator (consecutively/simultaneously/adsr/phase_mod/stacking_adsr/instruments) operates on this one shape via small per-channel helpers, so none of stereo_sound's oscillator/envelope math is duplicated a second time the way it was as a separate bundle.Waves ((t: number) => AsyncGenerator<void, number, undefined>) so that stepping/breakpoints and nested user closures evaluate correctly on the CSE machine.Wave-returning layer alongside the existingSoundlayer (sine_wave/square_wave/triangle_wave/sawtooth_wave/noise_wave/silence_wave), withsine_soundetc. as thin convenience wrappers.record()/record_for()use however many channels the input device actually has - a mono microphone (the common case) produces a Sound whose left and right channels are the same wave; no separaterecord_stereo.make_stereo_sound,play_waves,pan,pan_mod,squash,get_left_wave,get_right_wave(get_wavekeeps meaning "the" wave == left channel, so existing SICP-style code keeps working unchanged for the mono case).SoundModulePlugindeclares its ownchannelAttach, andmodules/src/tabs/SoundimplementsIPlugin/SoundTabRpcdirectly as the host-side counterpart (talking over Conductor'smakeRpchelper, now driving a real 2-channelAudioBuffer), matching the pattern confirmed against the rune migration (PR Migrate Rune Module #765) - no separate runner/web plugin package needed.plotly'sdraw_sound_2d, the only other consumer ofbundle-sound's types, for theWavetype's redesign.Draft while the tab-loading contract (loadTab/tabs, matching rune's PR #765) is still being finalized upstream.
Fixes #766 and #783
Test plan
tscclean forbundle-sound,tab-Sound, andbundle-plotlybundle-soundtests: 41/41 passingtab-Soundtests: 11/11 passingtscacross all bundles/tabs: clean