Migrate Rune Module by AaravMalani · Pull Request #765 · source-academy/modules

AaravMalani · 2026-07-06T12:02:05Z

Description

This PR migrates the rune module to become Conductor-ready. This includes adding serialisation and deserialisation methods and a plugin/channel transport mechanism. It also updates the buildtools documentation engine to support the functionDeclaration and variableDeclaration decorators for type inference.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Running the module on the Conductor-enabled Source Academy
Running yarn test with updated tests

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

gemini-code-assist

Code Review

This pull request refactors the rune module and its corresponding tab to use a plugin-based architecture integrated with @sourceacademy/conductor, enabling communication via serialized channel messages. Key performance feedback focuses on optimizing rendering loops: caching loaded textures directly on the Rune object to prevent redundant image loading and WebGL leaks, pre-deserializing animation frames in SerializedRuneAnimation to avoid high-frequency object creation, and replacing microtask chaining (.then()) in HollusionCanvas with synchronous execution of a resolved ref.

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.

gemini-code-assist · 2026-07-06T12:04:24Z

+class SerializedRuneAnimation extends glAnimation {
+  constructor(private readonly message: Extract<RuneDisplayMessage, { type: 'animation' }>) {
+    super(message.duration, message.fps);
+  }
+
+  getFrame(timestamp: number): AnimFrame {
+    if (this.message.frames.length === 0) {
+      return {
+        draw: new NormalRune(Rune.of()).draw
+      };
    }
-    return (
-      <WebGLCanvas
-        ref={(r) => {
-          if (r) {
-            rune.draw(r);
-          }
-        }}
-        key={elemKey}
-      />
+
+    const frame = Math.min(
+      Math.floor(timestamp * this.message.fps),
+      this.message.frames.length - 1
    );
+    const rune = deserializeRune(this.message.frames[frame]);
+    const drawnRune = this.message.mode === 'anaglyph'
+      ? new AnaglyphRune(rune)
+      : new NormalRune(rune);
+
+    return {
+      draw: drawnRune.draw
+    };
+  }
+}


In SerializedRuneAnimation.getFrame, deserializeRune is called on every single animation frame. This creates a new HTMLImageElement 60 times per second for textured runes, causing severe performance degradation and flickering. Deserializing all frames once in the constructor and caching them resolves this bottleneck.

class SerializedRuneAnimation extends glAnimation { private readonly deserializedFrames: Rune[]; constructor(private readonly message: Extract<RuneDisplayMessage, { type: 'animation' }>) { super(message.duration, message.fps); this.deserializedFrames = message.frames.map(deserializeRune); } getFrame(timestamp: number): AnimFrame { if (this.deserializedFrames.length === 0) { return { draw: new NormalRune(Rune.of()).draw }; } const frame = Math.min( Math.floor(timestamp * this.message.fps), this.deserializedFrames.length - 1 ); const rune = this.deserializedFrames[frame]; const drawnRune = this.message.mode === 'anaglyph' ? new AnaglyphRune(rune) : new NormalRune(rune); return { draw: drawnRune.draw }; } }

I don't agree with this, especially if there are 1000+ frames. I think the memory tradeoff isn't worth it
I'm leaving this up to the reviewer to decide

hmm. In practice, I've never really noticed significant performance degradation having the runes being drawn like this. I think this is okay, but we should probably move all rendering code into the tab eventually. From there if there are optimizations to be made then we can do that.

I also avoided memoizing the rendered runes so that you could have non-deterministic animations. We could decide we don't want that, but I think adding the memoization adds a layer of complexity that challenges how a cadet might expect a function to behave (i.e calling math_random multiple times ought to result in random behaviour)

In other words, if there's no significant impact to performance, I'm also okay to leave it this way,

leeyi45 · 2026-07-07T14:02:07Z

I am wondering if you've seen the migration to typedoc-plugin. Would that be something feasible to do? Then the editing of the declarations and signatures can occur as they are being created.

leeyi45 · 2026-07-07T13:56:52Z

I do implore you to add some explanation on what each function is trying to achieve here, and thus what kind of output is expected, or what each interface is intended to be

leeyi45 · 2026-07-07T14:04:18Z

+
+function parseFunctionDeclaration(paramTypes: string, returnType: string, sourceName: string): FunctionDeclarationMetadata {
+  const declarationSource = `function __sourceAcademyDocs__(${paramTypes}): ${returnType} {}`;
+  const sourceFile = ts.createSourceFile(`${sourceName}.functionDeclaration.ts`, declarationSource, ts.ScriptTarget.Latest, true);


Do we have to get Typescript to do the parsing? I mean it seems like we know all the details, so we could just populate the metadata object ourselves

Okay, so I'm looking at Typedoc, and according to a GH issue from a year ago

The support which TypeDoc removed was for parsing real decorators and removing them from the output, not the tag. It predated support for real decorators and was never actually used by any of TypeDoc's code.

So I think I'm gonna migrate to using an @public tag to provide the declaration info
Does that sound good?

If we want to use a tag, I think we should create our own custom tag to do it.

But what I meant was that it looks as if you're creating a sample program, then getting Typescript to parse the AST data when you could probably skip that step and just go from the paramTypes, returnType and sourceName straight to the output.

Yep, I've created a custom @publicType and @publicReturnType tag. Could you review the new pipeline?

leeyi45 · 2026-07-07T14:12:06Z

+  loadTab: (tab: string) => void;
+};
+
+const RUNE_CONSTANTS = [


I think this value should be defined by the actual rune constants and not just be defined on its own. You can probably get these values using Object.getOwnPropertyNames.

And then you can do some type shennenigans like:

type RuneConstantNames = { [K in keyof typeof RuneConstants as (typeof RuneConstants)[K] extends () => Rune ? K : never]: K } type RuneConstantFunctions = keyof RuneConstantFunctions;

Fair enough, I'll make the change

I think this value should be defined by the actual rune constants and not just be defined on its own. You can probably get these values using Object.getOwnPropertyNames.

And then you can do some type shennenigans like:

type RuneConstantNames = { [K in keyof typeof RuneConstants as (typeof RuneConstants)[K] extends () => Rune ? K : never]: K } type RuneConstantFunctions = keyof RuneConstantFunctions;

Are the type shenanigans even required? I could just iterate over the RuneFunctions and add the runes to the class. Even if they don't line up with the class declaration, its okay because the class variables are only used for documentation

leeyi45 · 2026-07-07T14:14:17Z

+        func as TypedValue<DataType.CLOSURE, DataType.OPAQUE>,
+        [{ type: DataType.NUMBER, value: timestamp }]
+      );
+      frames.push(serializeRune(await this.__getRune(result, funcName)));


This does kind of have me wondering, would it make sense to just render the entire animation at once with Promise.all?

I think that would cause a race condition, considering the CSE evaluator can only execute one function at a time.

A later frame could push to the CSE control during CSE execution, which would mess the execution up.

leeyi45 · 2026-07-07T14:28:34Z

+  method.signature = { args, returnType };
+}
+
+attachModuleMethod('anaglyph', [DataType.OPAQUE], DataType.OPAQUE);


I am always wary of cases where there are multiple sources of truth. In this case, there's some type games we can play to force the attachment call to match the definitions of the actual function:

type TypeToDataType<T> = T extends any[] ? T['length'] extends 2 ? DataType.PAIR | DataType.ARRAY : DataType.ARRAY : T extends (...args: any[]) => any ? DataType.CLOSURE : T extends boolean ? DataType.BOOLEAN : T extends null ? DataType.EMPTY_LIST : T extends number ? DataType.NUMBER : T extends string ? DataType.CONST_STRING : DataType.OPAQUE; type TupleToDataTypes<T extends any[]> = T extends [] ? [] : T extends [infer U, ...infer Rest] ? [TypeToDataType<U>, ...TupleToDataTypes<Rest>] : never; function attachModuleMethodNew<T extends (...args: any[]) => any>( f: T, args: TupleToDataTypes<Parameters<T>>, returnType: TypeToDataType<ReturnType<T>> ) { const methodName = f.name; const method = RuneModulePlugin.prototype[methodName] as SignaturedModuleMethod | undefined; if (method === undefined) { throw new Error(`Rune module method "${methodName}" does not exist.`); } method.signature = { args, returnType }; } attachModuleMethodNew(funcs.beside, [DataType.OPAQUE, DataType.OPAQUE], DataType.OPAQUE); attachModuleMethodNew(funcs.color, [DataType.OPAQUE, DataType.NUMBER, DataType.NUMBER, DataType.NUMBER], DataType.OPAQUE);

The helper types here probably need more nuance, and should honestly live outside of the rune bundle altogether, but you get the gist.

I guess the wider question would be if all of this can be avoided, but as an interim measure during this transition period it's okay.

Yep, I agree that replicating it twice is a pain, but I'm not convinced the tower of types is the best way.
Let me get back to you and see what is possible

I've moved the attachModuleMethod function to module-lib!

leeyi45 · 2026-07-07T14:37:01Z

   * When the image finished loading copy it into the texture.
   */
-  const loadTexture = (image: HTMLImageElement): WebGLTexture | null => {
+  const loadTexture = async (rune: Rune): Promise<WebGLTexture | null> => {


I would rather have it written like this:

let image: HTMLImageElement; if (typeof imageSource !== 'string') { image = imageSource; } else { image = await new Promise<HTMLImageElement>((resolve, reject) => { const image = Object.assign(new Image(), { crossOrigin: 'anonymous', src: imageSource }); image.onload = () => { rune.texture = image; resolve(image); }; image.onabort = reject; image.onerror = reject; }); }

than have an IIFE

Works! Let me change it

leeyi45 · 2026-07-07T14:53:12Z

+      if (renderFuncRef.current) {
+        return renderFuncRef.current(timestamp);
+      }
+      if (!isLoading.current) {


Is the isLoading ref necessary? It seems like we could just share the renderFuncRef. Something like

type RenderFuncState = 'loading' | ((time: number) => unknown)> | null; const renderFuncRef = React.useRef<RenderFuncState>(null);

Agreed, but I'd rather use { type: ...., value?: ... } the state instead

leeyi45 · 2026-07-07T14:56:14Z

+    return new NormalRune(rune);
+  }, [message, rune]);
+
+  if (drawnRune instanceof HollusionRune) {


Have you verified that this instanceof check works? Because of the way tabs and bundles are compiled this check might fail at runtime. See here

It does work. Earlier, the tabs and bundles shared the same object. Now since it's deserialised on the tab side with the tab's HollusionRune class, the instanceof check works

martin-henz · 2026-07-08T02:26:00Z

Let's avoid memoization until we have evidence that it's necessary.

Rules of Optimization:
Rule 1: Don't do it.
Rule 2 (for experts only): Don't do it yet.

AaravMalani added 2 commits July 3, 2026 01:52

feat: migrate rune

cb84fef

fix: update hollusion canvas to use promise based loading

ed4151d

gemini-code-assist Bot reviewed Jul 6, 2026

View reviewed changes

AaravMalani added 2 commits July 6, 2026 17:35

fix: restore curve's gl-matrix

8ed7ce5

fix: make fixes to rune and repeat

4037b56

AaravMalani marked this pull request as ready for review July 7, 2026 11:21

AaravMalani assigned leeyi45 Jul 7, 2026

leeyi45 reviewed Jul 7, 2026

View reviewed changes

AaravMalani added 4 commits July 13, 2026 03:50

feat: add type safety for attachModuleMethod

87f6c69

feat: add custom tags

659ea90

chore: lint files

ce94900

chore: make Lee Yi's changes

ff8ca72

AaravMalani requested a review from leeyi45 July 13, 2026 19:19

AaravMalani mentioned this pull request Jul 14, 2026

Migrate binary_tree module to Conductor #790

Open

Akshay-2007-1 mentioned this pull request Jul 15, 2026

Migrate midi module to Conductor #791

Open

Akshay-2007-1 added the Blocking label Jul 15, 2026

Uh oh!

Conversation

AaravMalani commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeyi45 Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leeyi45 commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-henz commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AaravMalani commented Jul 6, 2026 •

edited

Loading

leeyi45 Jul 7, 2026 •

edited

Loading