Migrate repl module to Conductor - #865
Conversation
Bundle side (src/bundles/repl) was already done in a prior interrupted session: index.ts/protocol.ts/rich_display.ts/stringify_value.ts implement the Conductor BaseModulePlugin, channel protocol, rich-text formatting and value stringification. This commit adds the missing host-side piece: the Repl tab (src/tabs/Repl) was still the legacy pre-Conductor implementation referencing the now-deleted programmable_repl.ts, so it has been rewritten as a Conductor IPlugin that subscribes to the repl channel, replays/relays output and program-text messages, and drives the AceEditor UI. Still to do: run lint/tsc/test for both packages and fix any fallout, then review the diff end-to-end (including a leftover unused REPL_RUNNER_ID export in protocol.ts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyGDAXkee7m9yRQSfHhVyZ
Bundle: - Remove dead REPL_RUNNER_ID export (unused anywhere, unlike the RUNNER_ID/WEB_ID pattern in matrix/sound's protocol.ts). - Fix a genuine unnecessary-assertion lint error and import ordering. - Add unit tests for the pure logic (rich_display.ts, stringify_value.ts). A test for ReplModulePlugin's decorated index.ts itself is not possible: vitest's esbuild-based SSR transform only lowers native (TC39 stage-3) decorators via its bundling API, not the single-file transform API it actually uses to load modules, so any file with an `@moduleMethod`- decorated async generator method fails with a raw SyntaxError on import. This is a pre-existing, documented limitation (see midi's conductorAdapters.ts) that already applies to sound/matrix/pix_n_flix - none of them test their decorated index.ts directly either. Tab: - Fix a non-exhaustive switch (explicitly no-op the tab->bundle 'run'/ 'request' message variants instead of a catch-all default) and two eslint-disable comments that turned out to guard against a rule that isn't actually enabled here. - Fix a test race: a localStorage write assertion needs to wait out the 100ms throttle window rather than checking synchronously. Verified tsc, lint, vitest and the real buildtools build pipeline are all green for both packages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyGDAXkee7m9yRQSfHhVyZ
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe REPL was migrated from standalone ChangesREPL Conductor migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReplView
participant ReplTabPlugin
participant REPLChannel
participant ReplModulePlugin
ReplView->>ReplTabPlugin: request code execution
ReplTabPlugin->>REPLChannel: publish run message
REPLChannel->>ReplModulePlugin: deliver run message
ReplModulePlugin->>ReplModulePlugin: execute registered evaluator
ReplModulePlugin->>REPLChannel: publish formatted output
REPLChannel->>ReplTabPlugin: deliver output
ReplTabPlugin->>ReplView: render output
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 7
🧹 Nitpick comments (4)
src/bundles/repl/src/index.ts (1)
303-330: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider guarding
__runCodeagainst concurrent runs.Each
runmessage starts a new evaluator call immediately. Two quick Run clicks interleave two generator drains, and their output messages interleave in__outputHistory. A simple in-flight flag keeps output ordered and prevents a second call while the first evaluator is still running.♻️ Proposed refactor
+ private __running = false; + private async __runCode(code: string): Promise<void> { + if (this.__running) return; if (!this.__evaluator) {Reset
__runningin afinallyblock around thetry.🤖 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/repl/src/index.ts` around lines 303 - 330, Update __runCode to guard against concurrent evaluator calls with an in-flight __running flag: return immediately when a run is already active, set the flag before starting evaluation, and reset it in a finally block so both success and error paths re-enable execution.src/bundles/repl/src/__tests__/rich_display.test.ts (1)
36-52: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a case for trailing characters after the hex colour.
The current cases do not detect the unanchored-regex problem reported on
src/bundles/repl/src/rich_display.tsLine 41. Add a case that rejects extra characters after the six hex digits.💚 Proposed test
test('rejects a colour missing digits', () => { expect(checkColorStringValidity('`#fff`')).toBe(false); }); + + test('rejects trailing characters after the hex digits', () => { + expect(checkColorStringValidity('`#ff0000`" onmouseover="alert(1)')).toBe(false); + });🤖 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/repl/src/__tests__/rich_display.test.ts` around lines 36 - 52, Add a test in the checkColorStringValidity suite that passes a six-digit hex color followed by extra characters and expects false, covering the trailing-character validation case without changing the existing cases.src/tabs/Repl/src/__tests__/index.test.ts (2)
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the restored text, not just the
getItemcall.
expect(freshPlugin).toBeDefined()and thegetItemspy pass even when the cached value is ignored. Assert that the restored text reaches the program state, for example by callingfreshPlugin.__runCode()on its own channel and checking therunpayload.💚 Proposed test
test('loads the initially cached program text from localStorage', () => { localStorageMock.setItem('programmable_repl_saved_editor_code', 'cached_code();'); - const freshPlugin = new ReplTabPlugin({} as any, [new MockChannel()], new MockTabService()); - expect(freshPlugin).toBeDefined(); - expect(localStorageMock.getItem).toHaveBeenCalledWith('programmable_repl_saved_editor_code'); + const freshChannel = new MockChannel<any>(); + const freshPlugin = new ReplTabPlugin({} as any, [freshChannel], new MockTabService()); + freshPlugin.__runCode(); + expect(freshChannel.sent).toContainEqual({ type: 'run', code: 'cached_code();' }); });🤖 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/tabs/Repl/src/__tests__/index.test.ts` around lines 101 - 106, Strengthen the test around ReplTabPlugin initialization by verifying that the cached editor text is restored into program state, not only that localStorage was queried. After creating freshPlugin, invoke its __runCode() using the test channel and assert the emitted run payload contains cached_code(); while retaining the existing localStorage interaction assertion.
108-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse fake timers instead of real sleeps.
Two tests wait 150 ms of wall-clock time each.
vi.useFakeTimers()withvi.advanceTimersByTime(150)removes the delay and the flake risk from timing.Also applies to: 136-147
🤖 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/tabs/Repl/src/__tests__/index.test.ts` around lines 108 - 113, Update the tests covering __setProgramText, including the additional test around lines 136-147, to use Vitest fake timers instead of awaiting real 150 ms sleeps. Enable fake timers for the test scope, advance them by 150 ms after triggering the throttled save, and restore real timers during cleanup while preserving the existing localStorage assertions.
🤖 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/repl/src/rich_display.ts`:
- Around line 40-42: Update checkColorStringValidity so its hexadecimal color
regex is anchored at both the start and end, allowing only exactly six
hexadecimal digits prefixed by # and rejecting any surrounding or trailing
content.
- Around line 49-107: Bound recursion in processRichDisplayContent using the
existing MAX_DEPTH convention from stringify_value.ts. Add and propagate a depth
parameter through recursive calls, and throw the established runtime error when
the maximum depth is exceeded, while preserving current string, pair, style, and
validation behavior.
In `@src/bundles/repl/src/stringify_value.ts`:
- Around line 29-47: Update stringifyReplValueDepth to track a shared total-node
budget in addition to MAX_DEPTH, decrementing it for each visited value and
returning the existing truncation marker when exhausted. Thread the same budget
through both head/tail recursion in the PAIR case and every recursive call in
the ARRAY case, so branching cycles cannot exceed the total traversal limit
while preserving the depth cap.
In `@src/tabs/Repl/src/__tests__/index.test.ts`:
- Around line 121-134: Update the test around __runCode so it actually verifies
that the two emitted output entries are accumulated, rather than only asserting
the run message. Expose the accumulated outputs through a read-only getter or
render the tab body, then assert that both “hello” and “world” entries are
present in order.
In `@src/tabs/Repl/src/index.tsx`:
- Around line 80-92: Guard both storage operations: update the constructor’s
localStorage.getItem access used to initialize programText and saveProgramText’s
localStorage.setItem call so SecurityError or quota failures are caught without
interrupting tab registration, REPL initialization, or the throttled save
callback. Preserve the existing value and save behavior when storage succeeds.
- Around line 115-120: Update ReplTabPlugin’s destroy() method to unsubscribe
its __handleMessage channel subscription while leaving the tab registration
intact. Ensure each Run removes the discarded plugin instance’s listener so it
no longer receives messages or mutates state after teardown.
- Around line 231-237: Update the AceEditor integration so changes to
state.fontSize are applied after mount, using an effect keyed on state.fontSize
or the component’s supported fontSize prop. Keep editorInstanceRef for the
editor instance, but ensure set_font_size triggers the mounted editor to receive
the new font size rather than applying it only in the ref callback.
---
Nitpick comments:
In `@src/bundles/repl/src/__tests__/rich_display.test.ts`:
- Around line 36-52: Add a test in the checkColorStringValidity suite that
passes a six-digit hex color followed by extra characters and expects false,
covering the trailing-character validation case without changing the existing
cases.
In `@src/bundles/repl/src/index.ts`:
- Around line 303-330: Update __runCode to guard against concurrent evaluator
calls with an in-flight __running flag: return immediately when a run is already
active, set the flag before starting evaluation, and reset it in a finally block
so both success and error paths re-enable execution.
In `@src/tabs/Repl/src/__tests__/index.test.ts`:
- Around line 101-106: Strengthen the test around ReplTabPlugin initialization
by verifying that the cached editor text is restored into program state, not
only that localStorage was queried. After creating freshPlugin, invoke its
__runCode() using the test channel and assert the emitted run payload contains
cached_code(); while retaining the existing localStorage interaction assertion.
- Around line 108-113: Update the tests covering __setProgramText, including the
additional test around lines 136-147, to use Vitest fake timers instead of
awaiting real 150 ms sleeps. Enable fake timers for the test scope, advance them
by 150 ms after triggering the throttled save, and restore real timers during
cleanup while preserving the existing localStorage assertions.
🪄 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 Plus
Run ID: 9282be2a-de28-4b4d-85ac-2f39db172aa3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
src/bundles/repl/package.jsonsrc/bundles/repl/src/__tests__/index.test.tssrc/bundles/repl/src/__tests__/rich_display.test.tssrc/bundles/repl/src/__tests__/stringify_value.test.tssrc/bundles/repl/src/config.tssrc/bundles/repl/src/functions.tssrc/bundles/repl/src/index.tssrc/bundles/repl/src/programmable_repl.tssrc/bundles/repl/src/protocol.tssrc/bundles/repl/src/rich_display.tssrc/bundles/repl/src/stringify_value.tssrc/bundles/repl/tsconfig.jsonsrc/tabs/Repl/index.tsxsrc/tabs/Repl/package.jsonsrc/tabs/Repl/src/__tests__/index.test.tssrc/tabs/Repl/src/index.tsxsrc/tabs/Repl/tsconfig.json
💤 Files with no reviewable changes (5)
- src/bundles/repl/src/config.ts
- src/bundles/repl/src/functions.ts
- src/tabs/Repl/index.tsx
- src/bundles/repl/src/tests/index.test.ts
- src/bundles/repl/src/programmable_repl.ts
martin-henz
left a comment
martin-henz
left a comment
There was a problem hiding this comment.
Review
Overall this is a solid, faithful migration of repl to Conductor — the wire protocol is clean, the tab correctly separates the "raw channel" pattern from the RPC pattern used elsewhere (matrix/sound) with a good rationale in the doc comment, and the new unit tests for rich_display.ts/stringify_value.ts are thorough. One real functional bug and a pre-existing (not introduced here) security issue worth flagging below.
Correctness
set_background_image/set_font_size/set_program_text never trigger the lazy tab load (src/bundles/repl/src/index.ts)
__displayOutput (used by repl_display/rich_repl_display/__runCode) calls this.__loadReplTab(), but __displayEditorProps (used by set_background_image/set_font_size) and set_program_text do not:
private __displayEditorProps(): void {
const message: ReplEditorPropsMessage = { type: 'editor_props', ...this.__editorProps };
this.__latestEditorProps = message;
if (this.__tabRequested) {
this.__replChannel.send(message);
}
}async* set_program_text(text: TypedValue<DataType.CONST_STRING>): AsyncGenerator<...> {
const message: ReplSetProgramTextMessage = { type: 'set_program_text', text: text.value };
this.__latestProgramText = message;
if (this.__tabRequested) {
this.__replChannel.send(message);
}
return mVoid();
}Neither calls __loadReplTab(). A program whose only interaction with the module is set_program_text(...) — a very plausible use case per its own doc comment ("Set program text in the Repl editor to the given string", i.e. pre-populating starter code for a student) — will never cause the Repl tab to actually open. The text is latched (__latestProgramText) and would only reach the tab if something else happens to open it later, which defeats the point of pre-populating starter code for the student to immediately see and run.
Suggest calling this.__loadReplTab() at the top of __displayEditorProps() and inside set_program_text(), matching __displayOutput's behavior and the established convention elsewhere in this codebase (e.g. matrix's get_matrix()/clear_matrix() both call this.__ensureTabLoaded(), sound's play()/init_record() etc. all do too) — every host-touching entry point calls the lazy-load helper, not just one of them.
Security (pre-existing, not introduced by this PR)
checkColorStringValidity in rich_display.ts uses an unanchored regex:
export function checkColorStringValidity(htmlColor: string): boolean {
return /#[0-9a-f]{6}/u.test(htmlColor.toLowerCase());
}The clrt/clrb colour value (colorHex = config_str.substring(4)) is validated with this and then interpolated directly into the style="..." attribute that the tab renders via dangerouslySetInnerHTML. Since the regex isn't anchored, a payload like:
rich_repl_display(pair("text", "clrt#ff0000\" onmouseover=\"alert(1)"));passes validation (it contains # + 6 hex digits) and breaks out of the style attribute, injecting an arbitrary attribute/event handler — a DOM XSS via rich_repl_display. Note xssStringCheck (which blocks <, >, script, etc.) is only ever applied to the base/leaf text content, never to the tail/style string, so nothing else catches this either.
I confirmed this is byte-for-byte identical to the deleted programmable_repl.ts's checkColorStringValidity/processRichDisplayContent, so it's not introduced by this migration. But since this PR is the first time this file has been touched in a while, it might be worth either fixing it here (anchor the regex: /^#[0-9a-f]{6}$/u) or filing a follow-up issue so it doesn't get forgotten again.
Test coverage
src/tabs/Repl/src/__tests__/index.test.ts's 'an output message from the bundle is accumulated' test doesn't actually assert anything about the two emitted entries:
test('an output message from the bundle is accumulated', () => {
channel.emit({ type: 'output', entry: { content: 'hello', ... } });
channel.emit({ type: 'output', entry: { content: 'world', ... } });
// Output isn't exposed via a getter - re-running the code and checking the channel traffic
// exercises the same accumulation logic without reaching into private state.
plugin.__runCode();
expect(channel.sent).toContainEqual({ type: 'run', code: '' });
});Since ReplViewState.outputs isn't exposed via any getter, this only proves __runCode() still works — it would pass even if the 'output' case in __handleMessage were completely broken (e.g. never pushed to outputs at all). Consider exposing a small test-only accessor (SoundTabPlugin.getStatus() is the precedent for this in the codebase) so the actual accumulated state can be asserted directly.
Minor / nits (both pre-existing, faithfully ported — not blocking)
set_font_size's doc comment says "@param font_size_pxfont size (in pixels)", but the tab applies it as`${state.fontSize}pt`(points, not pixels) viaeditorInstance.setOptions(...). 1pt ≈ 1.33px, so rendered size doesn't match the documented unit. Identical to the deleted pre-Conductor tab, so not new here.- In
ReplView, theAceEditorrefcallback is a fresh arrow function every render, sosetOptions({fontSize})re-runs on every re-render rather than just on mount/fontSize change. Harmless (idempotent) but redundant —react-ace'sAceEditoralso accepts afontSizeprop directly, which would sidestep the manualsetOptionscall entirely. Again, faithfully ported from the old implementation's pattern.
What's good
- The doc comment explaining why
default_js_slangcan't work under Conductor (and pointing at the exactcommons.tsline that marksjs-slang*external) is excellent — this is exactly the kind of context a future reader needs. - Choosing a raw subscribe/send channel over
makeRpcfor this module, with a clear one-paragraph justification, shows real understanding of when each pattern in this codebase applies (matchespix_n_flix's frame channel precedent). stringify_value.ts's recursion depth guard (and the test for it against a self-referential pair) is a nice defensive touch that the old implementation didn't need to worry about in the same way.- Backlog-replay semantics (
__tabRequestedgate, replaying full history vs. latest-only for props/program-text) are well thought through and well tested.
- rich_display.ts: anchor checkColorStringValidity's regex (was unanchored, letting a crafted clrt#/clrb# tail break out of the style="..." attribute the tab renders via dangerouslySetInnerHTML - a real DOM XSS via rich_repl_display); bound processRichDisplayContent's recursion so a cyclic pair (set_head/set_tail) can't recurse forever. - stringify_value.ts: add a shared total-node budget alongside the existing depth cap - a pair whose head AND tail both cycle back branches at every level, so depth alone still allows up to 2^MAX_DEPTH calls. - index.ts: guard __runCode against two overlapping runs (e.g. a fast double-click on Run) driving the same evaluator closure concurrently. - tabs/Repl/index.tsx: guard localStorage access (throws in some private-browsing/quota-exceeded contexts); unsubscribe from the channel in destroy() so a discarded plugin instance stops reacting to messages; move fontSize application into an effect keyed on state.fontSize instead of only the AceEditor ref callback, and add a getOutputs() accessor for tests. - Tests: add regression coverage for all of the above, strengthen two tests that weren't actually asserting what their names claimed, and switch two throttle-wait tests to fake timers instead of real 150ms sleeps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyGDAXkee7m9yRQSfHhVyZ
|
Thanks for the thorough review — went through all 7 actionable comments (replied individually on each thread) plus the 4 nitpicks, in 4ae6507:
Full rundown of the 7 actionable fixes is in my replies on each thread. tsc/lint/vitest/ |
- index.ts: set_background_image/set_font_size/set_program_text now call __loadReplTab(), matching __displayOutput's behavior. Previously, a program whose only interaction with the module was one of these three - most plausibly set_program_text, used to pre-populate starter code for a student - would never cause the Repl tab to actually open, silently defeating the feature. - tabs/Repl/index.tsx: apply font size in px, not pt, to actually match set_font_size's own doc comment and parameter name (font_size_px) - pre-existing mismatch carried over from the deleted pre-Conductor tab. No corresponding bundle-side test: importing index.ts directly in a test still hits the same vitest/decorator limitation noted elsewhere in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyGDAXkee7m9yRQSfHhVyZ
|
Handled both findings from this review, in 5ba98cd:
The security finding (unanchored colour regex) and the weak test-coverage finding from this same review were already handled as part of CodeRabbit's review (which flagged the same two issues independently) — see 4ae6507 and my replies on that review's threads. |
set_evaluator() was the one repl function that gave a program a reason to use the tab but never actually triggered the lazy tab loader - every other producing/customizing function (set_program_text, repl_display, set_background_image, ...) already calls __loadReplTab(), but set_evaluator() was missed when the lazy-load pattern was introduced in the Conductor migration (#865). As a result, a program whose only interaction with the module is set_evaluator() - exactly the module's own documented minimal usage example - never opened the Repl tab at all. Claude-Session: https://claude.ai/code/session_01B3LRoXRMQ5ADeYMdVTChCZ Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
replbundle (src/bundles/repl) from the legacy js-slang module pipeline to Conductor'sBaseModulePlugin/channel protocol, following the same pattern as the matrix/sound/pix_n_flix/csg migrations.Repltab (src/tabs/Repl) from the legacydefineTab/getModuleStateAPI to a ConductorIPluginthat subscribes to the repl channel, replays/relays output and program-text messages, and drives the AceEditor UI (Run button, draggable editor height, plaintext/rich-text output history).default_js_slang(previously ran Repl input directly through js-slang, reusing the enclosing program's context) is no longer supported under Conductor —js-slang*imports resolve toundefinedat runtime in a Conductor bundle's Worker, the same problemcsghit. The function is kept as an exported symbol (soset_evaluator(default_js_slang)still type-checks) but now throws a clear error directing the caller to write their own evaluator viaset_evaluator.rich_display.ts's XSS filtering/colour validation/nested rich-text parsing,stringify_value.ts's value stringification and recursion guard) and for the tab's channel wiring (backlog replay, localStorage caching, run dispatch). A test that imports the bundle's decoratedindex.tsdirectly isn't possible: vitest's esbuild-based SSR transform only lowers native (TC39 stage-3) decorators via its bundling API, not the single-file transform it actually uses to load modules, so any file with an@moduleMethod-decorated async generator method throws a rawSyntaxErroron import. This is a pre-existing, already-documented limitation (seemidi/src/conductorAdapters.ts) that applies equally tosound/matrix/pix_n_flix— none of them test their decoratedindex.tsdirectly either.Test plan
tsc,lint, andvitestall pass for bothsrc/bundles/replandsrc/tabs/Replbuildtools build(the real production pipeline, not justtsc) succeeds for both packagesrepl_display()/rich_repl_display()/run, the editor → Run → evaluator-closure → output round trip works, the "no evaluator registered" warning renders as rich text, a thrown evaluator error renders in red, and the editor's cached program text persists across separate Runs vialocalStorage🤖 Generated with Claude Code
https://claude.ai/code/session_01PyGDAXkee7m9yRQSfHhVyZ