Migrate Matrix (Tone Matrix) bundle to Conductor#802
Conversation
…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}
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
|
Is sound_matrix being used? I thought the intention was for it to be superseded by a proper matrix module |
This is the proper matrix module. "sound_matrix" is a misnomer: this module has nothing to do with sound. So let's rename it to just "matrix". |
Yes we have been using a version of this that was (and still is) hardwired into the frontend. Once this is properly deployed we can clean up the frontend. |
Akshay-2007-1
changed the title
I have renamed it to just "Matrix (Tone Matrix)" |
…igrate-sound-matrix # Conflicts: # yarn.lock
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, SOUND_MATRIX_CHANNEL_ID -> MATRIX_CHANNEL_ID, SOUND_MATRIX_WEB_ID -> MATRIX_WEB_ID, SOUND_MATRIX_TAB_ID -> MATRIX_TAB_ID, SOUND_MATRIX_TAB_NAME -> MATRIX_TAB_NAME - Runtime string values: channel id, web id, globalThis storage key symbol, error messages, all updated to match - Student-facing tab label: 'Sound Matrix' -> 'Tone Matrix' (matches the quest's own official name, "The Magical Tone Matrix") - @module JSDoc tags, comments, and manifest.json's tab reference Left untouched, deliberately: genuine references to "CS1101S Mission 15, Sound mission" (the actual course mission name this quest belongs to - a factual identifier, not a code-naming inconsistency) and the standing `sound` module import of playback functions the Tone Matrix genuinely calls at the Python/Source level (unrelated to this bundle's own identity). Verified via a full case-insensitive repo-wide grep for sound_matrix/soundmatrix/SoundMatrix: zero remaining hits anywhere, inside or outside these two directories, after the rename. tsc/lint/ test (15/15 across bundle+tab)/build/compile/docs-build all clean for both packages post-rename. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…igrate-sound-matrix # Conflicts: # yarn.lock
|
I see. I have done previous work to try and create a new matrix module, we won't be using that? Or should we merge the efforts? |
The way I see it: This IS your SoundMatrix, just renamed and conductor-enabled. How did it turn out? |
…igrate-sound-matrix
This current implementation differs from mine in that it is missing:
The It's more angled as a generic matrix component, rather than specially designed for use with |
3 participants
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