rune: constructor/initialise()/tab-lookup nitpicks + missing protocol round-trip test (from csg review)

[martin-henz]

Summary

CodeRabbit's review of the csg migration (#826) flagged several small correctness/quality issues. Checked which of those also apply to rune's current code (src/bundles/rune/src/index.ts), since the two modules share the same constructor/initialise()/tab-loading shape. Four do; the rest are CSG/jscad-specific (WebGL renderer internals, STL download handling) or don't apply to rune's message shape, so they're not listed here.

Not urgent, no user-facing symptom currently observed - opening this so the improvements aren't lost, not because anything is on fire. Related but distinct from already-filed #823 (duplicate delivery) and #825 (js-slang-backed errors resolving to undefined under Conductor) - this issue is about the four items below only.

1. Constructor validates the channel after calling super()

// src/bundles/rune/src/index.ts:184-198
constructor(
  conduit: IConduit,
  [runeChannel]: IChannel<any>[],
  evaluator: IInterfacableEvaluator,
  tabLoader: RuneTabLoader
) {
  super(conduit, [runeChannel], evaluator);

  if (!runeChannel) {
    throw new GeneralRuntimeError('Rune channel is required but was not provided.');
  }
  ...

super(conduit, [runeChannel], evaluator) already hands BaseModulePlugin an [undefined] channels array before the guard ever runs. A throw is legal before super() as long as this isn't touched, so the check should move above the super(...) call - same fix applied to csg in #826.

(Separately: GeneralRuntimeError here is the legacy js-slang-backed class #825 already flags as resolving to undefined under Conductor - that's tracked there, not repeated here. Fixing the ordering doesn't require also fixing the error class, but doing both together avoids two round-trips through this constructor.)

2. initialise() isn't idempotent

// src/bundles/rune/src/index.ts:200-213
override async initialise() {
  await super.initialise();
  for (const name in funcs.RuneFunctions) {
    const value = funcs.RuneFunctions[name as keyof typeof funcs.RuneFunctions];
    if (!(value instanceof Rune)) continue;
    (this as unknown as Record<string, Rune>)[name] = value;
    this.exports.push({ symbol: name, value: await this.__makeRune(value) });
  }
}

A second call re-pushes every primitive Rune constant onto exports again. Confirmed directly (not just by inspection) while fixing the identical issue in csg: BaseModulePlugin.initialise() itself isn't idempotent either, so a guard needs to wrap the whole method, including the super.initialise() call, not just the loop - a guard around only the loop still lets super.initialise() re-push every method export on a second call. csg's fix (#826):

override async initialise() {
  if (this.__initialised) return;
  this.__initialised = true;

  await super.initialise();
  // ... rest unchanged
}

3. Tab lookup by position instead of by name

// src/bundles/rune/src/index.ts:219-228
private __loadRuneTab(): boolean {
  if (this.__tabLoaded || this.__tabLoader === undefined) return true;

  const tabName = this.__tabLoader.tabs[0];
  if (tabName === undefined) return true;
  ...

protocol.ts already exports RUNE_TAB_NAME = 'Rune'. Using this.__tabLoader.tabs.find(tab => tab === RUNE_TAB_NAME) instead of tabs[0] makes the lookup explicit and fails loudly if the tab is ever missing, rather than silently trusting position. Functionally identical today (rune only ships one tab), same low-priority reasoning as csg's version of this fix.

4. No protocol-level round-trip test

src/bundles/rune/src/protocol.ts exports serializeRune; the tab (src/tabs/Rune/src/index.tsx) defines its own local deserializeRune rather than a shared one from protocol.ts. Neither has a dedicated test that round-trips a Rune through serializeRune/deserializeRune and asserts the result matches - src/bundles/rune/src/__tests__/index.test.ts covers the pure colour/composition functions and one Conductor-closure-binding test, but nothing on the wire format itself. This is actually a bigger gap than csg had before #826 (csg at least had a bounding-box-only check; rune has none), for the same reason csg's was worth strengthening: a serialization bug (dropped field, wrong array order) wouldn't be caught by the existing tests.

Suggested fix

Same shape as the corresponding fixes in #826:

• Move the channel guard above super() in the constructor.
• Add an __initialised guard around the whole of initialise().
• Import RUNE_TAB_NAME from ./protocol and use it in __loadRuneTab.
• Add a src/bundles/rune/src/__tests__/protocol.test.ts (or extend the existing test file) that builds a Rune, round-trips it through serializeRune/a shared deserializeRune, and asserts full structural equality - may be worth moving deserializeRune into protocol.ts so both the bundle's test and the tab's real usage share one implementation, rather than the tab keeping its own private copy.

[11suixing11]

Hi, I can take a look at this.

I’ll target the conductor-migration branch as described in the issue thread, keep the PR scoped to the four rune items listed here, and add/adjust tests for the idempotent initialisation and protocol round-trip behavior where the repo structure supports it.

[martin-henz]

Thanks a lot for contributing!