Wire autocomplete and syntax highlighting to ace editor#3828
Wire autocomplete and syntax highlighting to ace editor#3828
Conversation
Set up the conductor autocomplete plugin host side, including per-evaluator syntax highlighting via SyntaxHighlightData. Fixes startup highlighting by listening to editor session swaps (changeSession) and reapplying the correct ace mode. Eliminates duplicate conductor preload race by consolidating preload to a single saga handler. Co-authored-by: Aarav Malani <aarav.malani@gmail.com>
MengJit
changed the title
![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 integrates the Conductor framework for code evaluation and autocomplete, adding the @sourceacademy/autocomplete dependency and implementing an AutocompletePlugin. It introduces a caching mechanism for evaluators to reduce startup latency and updates the Editor and WorkspaceSaga to support these features. The review highlights the need for consistent indentation in WorkspaceSaga, suggests making the AutocompletePlugin language-agnostic by removing hardcoded imports, and advises ensuring unique IDs in ace.define to prevent potential collisions.
| import { require as acequire } from 'ace-builds/src-noconflict/ace'; | ||
| import ace from 'ace-builds/src-noconflict/ace'; | ||
| import { EventChannel, eventChannel, Unsubscribe } from 'redux-saga'; | ||
| import 'ace-builds/src-noconflict/mode-python'; |
There was a problem hiding this comment.
Hardcoding the import for mode-python in a generic AutoCompletePlugin seems incorrect. This plugin should ideally be language-agnostic, and specific modes should be loaded dynamically or provided by the evaluator.
There was a problem hiding this comment.
Agreed that hardcoding mode-python is not ideal for a fully language-agnostic plugin. However, this import is required because the runner's SyntaxHighlightData uses ace's existing Python mode helpers via hookFrom references (ace/mode/python, ace/mode/folding/pythonic) for indentation and folding rules. Without this import, acequire('ace/mode/python') returns undefined at runtime and loadMode throws.
A cleaner long-term solution would be for the runner to include all folding/indent logic as primitive regex rules in SyntaxHighlightData rather than referencing pre-existing ace modes via hookFrom, making the host truly language-agnostic. That is tracked as a future improvement. For now, this import is the minimal fix to make the feature work.
| ace.define( | ||
| data.id, | ||
| ['exports', 'module'], | ||
| function (require: never, exports: { Mode: typeof Mode }) { | ||
| exports.Mode = Mode; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Using ace.define with data.id might lead to collisions if multiple evaluators use the same ID or if the ID is not sufficiently unique. Ensure that data.id is always unique per evaluator instance.
There was a problem hiding this comment.
data.id is guaranteed unique per evaluator by construction. It is set in the runner's mode.ts as ace/mode/${evaluatorName} where evaluatorName is the bundle name (e.g. PyCseEvaluator1, PyCseEvaluator2, etc.), passed explicitly through the constructor chain from each evaluator subclass. Since each bundle is a distinct worker with its own evaluator name hardcoded at build time, two evaluators cannot produce the same data.id. If the same evaluator is reloaded (e.g. language switch back), ace.define with the same id is idempotent — ace simply overwrites with an identical definition, which is harmless.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Coverage Report for CI Build 25487350750Coverage decreased (-0.3%) to 41.019%Details
Uncovered Changes
Coverage Regressions3 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
Remove the unused commented code
There was a problem hiding this comment.
Pull request overview
Adds host-side wiring for Conductor-backed autocomplete and dynamic Ace syntax highlighting modes, enabling per-evaluator highlighting (e.g. ace/mode/PyCseEvaluator1) and using a preloaded Conductor instance to reduce latency.
Changes:
- Add
@sourceacademy/autocompletedependency and register a new host-sideAutoCompletePluginwith Conductor. - Route editor autocomplete through the Conductor runner (with a timeout) when
conductor.enableis on. - Introduce a Conductor evaluator preload/cache helper and update the editor to re-apply the Ace mode on
changeSession.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Adds lockfile entry for @sourceacademy/autocomplete GitHub dependency. |
| package.json | Adds @sourceacademy/autocomplete dependency pin. |
| src/features/conductor/createConductor.ts | Registers the new autocomplete/syntax-highlighting plugin on the host side. |
| src/features/conductor/AutocompletePlugin.ts | Implements host-side plugin that registers Ace modes from SyntaxHighlightData and exposes complete(). |
| src/commons/sagas/WorkspaceSaga/index.ts | Uses Conductor plugin for autocomplete requests when enabled. |
| src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts | Switches evaluation to use the preloaded Conductor instance. |
| src/commons/sagas/LanguageDirectorySaga.ts | Preloads the Conductor evaluator when a language is selected. |
| src/commons/sagas/helpers/conductorEvaluatorCache.ts | New helper that caches/preloads a Conductor instance for the selected evaluator. |
| src/commons/editor/Editor.tsx | Applies dynamic Ace modes on session changes while Conductor is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!modeModule?.Mode) return false; | ||
| if ((session.getMode() as any).$id === modeId) return true; | ||
| session.setMode(new modeModule.Mode()); | ||
| return true; | ||
| }; | ||
|
|
||
| const attachToSession = (session: any) => { |
| if (yield select(selectConductorEnable)) { | ||
| const { conduit }: { hostPlugin: BrowserHostPlugin; conduit: IConduit } = | ||
| yield call(getPreparedConductorSaga); | ||
|
|
||
| const plugin = conduit.lookupPlugin('__autocomplete_plugin_web') as AutoCompletePlugin; | ||
| if (plugin) { | ||
| const channel: EventChannel<AutoCompleteEntry[]> = yield call( | ||
| [plugin, 'complete'], | ||
| autocompleteCode, | ||
| action.payload.row + prependLength, | ||
| action.payload.column | ||
| ); | ||
| //const names: AutoCompleteEntry[] = yield take(channel); | ||
| const { names, timeout }: { names?: AutoCompleteEntry[]; timeout?: true } = yield race({ | ||
| names: take(channel), | ||
| timeout: delay(3000) | ||
| }); | ||
|
|
||
| if (timeout || !names) { | ||
| console.warn('autocomplete channel timed out — runner never replied'); | ||
| channel.close(); | ||
| return; | ||
| } | ||
|
|
||
| yield call( | ||
| action.payload.callback, | ||
| null, | ||
| names.map(name => ({ | ||
| meta: name.meta, | ||
| value: name.name, | ||
| caption: name.name, | ||
| docHTML: name.docHTML, | ||
| score: name.score ? name.score + 1000 : 1000, // Prioritize suggestions from code | ||
| name: undefined | ||
| })) | ||
| ); | ||
| channel.close(); | ||
| } | ||
| return; | ||
| } |
| // A new evaluator path is requested, so release the old preloaded conductor first. | ||
| yield call(cleanupPreparedConductorSaga); | ||
|
|
||
| loadingConductorPath = path; | ||
| loadingConductorPromise = createPreparedConductor(path) | ||
| .then(prepared => { | ||
| preparedConductorPath = path; | ||
| preparedConductor = prepared; | ||
| return prepared; | ||
| }) | ||
| .finally(() => { | ||
| loadingConductorPath = null; | ||
| loadingConductorPromise = null; | ||
| }); | ||
|
|
||
| return yield call(() => loadingConductorPromise as Promise<PreparedConductor>); | ||
| } |
| if (files) { | ||
| prepared.setFiles(files); | ||
| } | ||
|
|
||
| // Consume only when requested (e.g. for program evaluation, not autocomplete requests). | ||
| if (consume && preparedConductor === prepared) { | ||
| resetPreparedConductor(); | ||
| } | ||
|
|
| export function* getPreparedConductorSaga( | ||
| options?: GetPreparedConductorOptions | ||
| ): SagaIterator<{ hostPlugin: BrowserHostPlugin; conduit: IConduit }> { | ||
| if (!currentEvaluatorPath) { | ||
| throw Error('no evaluator path selected'); | ||
| } | ||
|
|
||
| const path = currentEvaluatorPath; | ||
| const prepared: PreparedConductor = yield call(ensurePreparedConductorSaga, path); | ||
| const files = options?.files; |
| [LanguageDirectoryActions.setSelectedLanguage.type]: function* () { | ||
| const language = yield call(getLanguageDefinitionSaga); | ||
| if (!language) return; | ||
| if (language.evaluators.length > 0) { | ||
| yield put(LanguageDirectoryActions.setSelectedEvaluator(language.evaluators[0].id)); | ||
| } | ||
|
|
||
| const conductorEnabled: boolean = yield select(selectConductorEnable); | ||
| if (!conductorEnabled) return; | ||
|
|
||
| const evaluator = yield call(getEvaluatorDefinitionSaga); | ||
| if (!evaluator?.path) return; | ||
|
|
||
| try { | ||
| yield call(preloadConductorEvaluatorSaga, evaluator.path); | ||
| } catch (error) { | ||
| console.error('Failed to preload:', error); | ||
| } | ||
| } |
| this.autocomplete(code, row, column, entries => { | ||
| emit(entries.declarations); | ||
| }); | ||
| return () => {}; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@MengJit I think some of the comments may be valid, please check and resolve if invalid, thanks! |
Description
Sets up the conductor autocomplete plugin on the host side, including per-evaluator syntax highlighting via SyntaxHighlightData. Each evaluator registers its own ace mode id (e.g. ace/mode/PyCseEvaluator1) which the editor session subscribes to dynamically.
Key changes:
Paired PR: source-academy/py-slang#109
Co-authored-by: Aarav Malani aarav.malani@gmail.com
Type of change
How to test
conductor.enablefeature flag in the app and set the directory accordingly. A sample directory.json file has been attached.def,return,if) should be highlighted immediately on page load without needing to switch evaluatorsChecklist
directory.json