Migrate sound_matrix (Tone Matrix) bundle to Conductor#802
Open
Akshay-2007-1 wants to merge 13 commits into
Open
Migrate sound_matrix (Tone Matrix) bundle to Conductor#802Akshay-2007-1 wants to merge 13 commits into
Akshay-2007-1 wants to merge 13 commits into
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…flict, yarn quarantine)
…destroy() unregistering too eagerly)
…s, not be per-instance
…sn'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.
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.
![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 ports the sound_matrix module to Conductor, splitting it into a runner-side plugin and a host-side tab plugin that communicate via RPC over a dedicated channel. Key feedback includes addressing a useSyncExternalStore contract violation caused by mutating the shared matrix in-place, implementing the destroy lifecycle method in SoundMatrixModulePlugin to clear active timeouts and prevent memory leaks, and defensively checking for a null canvas context in __setColor.
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.
…y 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>
…ssing 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>
…ontext
- __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>
…igrate-sound-matrix # Conflicts: # yarn.lock
Contributor
|
Is sound_matrix being used? I thought the intention was for it to be superseded by a proper matrix module |
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
2 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
Migrates the
sound_matrixbundle (Quest Q5B, "The Magical Tone Matrix") to Conductor, following the same pattern as the recentsound/midi/binary_treemigrations.Fixes #787
sound_matrixbundle:get_matrix,clear_matrix,set_timeout,clear_all_timeoutreimplemented as@moduleMethod-decoratedBaseModulePluginmethods, talking to a new host-side tab over Conductor's RPC channel instead of touching the DOM directly (module code runs in the Conductor Worker, not the main thread).SoundMatrixtab: owns the actual 16x16 grid state and canvas rendering (click-to-toggle), replies to the bundle's RPC calls (getMatrix,clearMatrix).get_matrix()keeps the original row-0-is-bottom-row convention (verified against the actual Quest Q5B spec: rows are counted from the bottom).Tested manually end-to-end against a live frontend + both the CSE machine and the (not yet merged) PVML-in-browser evaluator for SICPy, combined with the
soundmodule - a full Tone Matrix pattern combiningsimultaneously()'d columns with aset_timeout-drivensequence()loop plays correctly on both engines.Test plan
yarn tscclean (bundle + tab)yarn lintclean (bundle + tab)yarn testpassing (bundle: 6 tests, tab: 8 tests)yarn buildsucceeds (bundle + tab)yarn buildtools build docssucceedsyarn constraintscleanget_matrix()row/column convention matches Quest Q5B,set_timeout/clear_all_timeoutdrive a full playback sequence correctly