Add Dynamic Tab and Plugin Loading#3977
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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 introduces dynamic plugin loading and tab management by integrating @sourceacademy/common-tabs and introducing a SideContentManager (TabService) to handle dynamic tab registration, visibility, and rendering. It also updates the conductor evaluation flow to support loading external plugins dynamically. The code review feedback highlights several key improvements: first, tabs should not be cleared immediately upon evaluation or conductor termination (as this causes them to disappear prematurely), but rather at the start of a new evaluation; second, newly registered tabs should default to being visible; and finally, the subscribe method in TabService should be defined as an arrow function to avoid inefficient re-binding and re-subscription cycles in SideContentProvider.
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.
| yield call([conduit, 'terminate']); | ||
| yield call([sideContentManager, sideContentManager.clearTabs]); | ||
| yield cancel(stdoutTask); |
There was a problem hiding this comment.
Clearing the tabs immediately when the evaluation finishes or is terminated will cause any dynamically loaded tabs (such as visualization or interactive game tabs) to disappear as soon as the program stops running.
To allow users to interact with and view the tabs after the program execution completes, the tabs should persist. Instead of clearing them here at the end of the evaluation, you should clear them at the beginning of evalCodeConductorSaga when a new execution is initiated.
| yield call([conduit, 'terminate']); | |
| yield call([sideContentManager, sideContentManager.clearTabs]); | |
| yield cancel(stdoutTask); | |
| yield call([conduit, 'terminate']); | |
| yield cancel(stdoutTask); |
There was a problem hiding this comment.
That isn't true, it doesn't immediately vanish. It does reload when the run button is pressed, so maybe finding a way to persistently get run the Conductor across evaluations is something to consider
| subscribe(listener: Listener): () => void { | ||
| this.listeners.add(listener); | ||
| return () => this.listeners.delete(listener); | ||
| } |
There was a problem hiding this comment.
Defining subscribe as a standard method and then binding it on every render in SideContentProvider (via sideContentManager.subscribe.bind(sideContentManager)) creates a new function reference on every render. This causes useSyncExternalStore to unsubscribe and re-subscribe on every single render, which is highly inefficient.
To fix this, define subscribe as an arrow function property in SideContentManager. This automatically binds the method to the instance and ensures a stable function reference, allowing you to pass sideContentManager.subscribe directly without .bind.
| subscribe(listener: Listener): () => void { | |
| this.listeners.add(listener); | |
| return () => this.listeners.delete(listener); | |
| } | |
| subscribe = (listener: Listener): () => void => { | |
| this.listeners.add(listener); | |
| return () => this.listeners.delete(listener); | |
| }; |
| const serviceTabs = useSyncExternalStore( | ||
| sideContentManager.subscribe.bind(sideContentManager), | ||
| () => sideContentManager.getTabs(workspaceLocation), | ||
| ); |
There was a problem hiding this comment.
Now that subscribe is defined as a stable arrow function on sideContentManager, you can pass it directly to useSyncExternalStore without calling .bind(). This avoids creating a new function reference on every render and prevents unnecessary unsubscriptions/resubscriptions.
| const serviceTabs = useSyncExternalStore( | |
| sideContentManager.subscribe.bind(sideContentManager), | |
| () => sideContentManager.getTabs(workspaceLocation), | |
| ); | |
| const serviceTabs = useSyncExternalStore( | |
| sideContentManager.subscribe, | |
| () => sideContentManager.getTabs(workspaceLocation), | |
| ); |
1 participant
Description
This PR helps add dynamic loading of tabs from external plugins. It provides an implementation of the
ITabServicepassed to web plugins when plugins are initially loaded, creating asideContentManagerwhich emits tab changes to the required workspaces.It also adds a plugin registry which stores installable plugins implemented in the frontend (autocomplete, etc.). It utilises the plugin directory to load external plugins if an installable plugin is not currently present with a given ID.
Type of change
How to test
Checklist