fix: ensure terminal API object identity and respect grouping mode for plugin-created terminals by ndoschek · Pull Request #17340 · eclipse-theia/theia

ndoschek · 2026-04-10T14:12:09Z

What it does

The ms-python extension uses strict instance equality (===) in its terminalCloseHandler to check whether a terminal has been closed. However, Theia wraps terminal objects in a Proxy via createAPIObject, so the object returned to extensions differs from the one fired in terminal events. This causes the equality check to fail, making the "Run Python File" button unresponsive after closing and reopening a terminal.

Terminal events (onDidOpenTerminal, onDidCloseTerminal, onDidChangeTerminalState) now fire with the same Proxy-wrapped API object that was returned to extensions
The terminals getter and activeTerminal also return the API objects
A _terminalApiObjects map in TerminalServiceExtImpl tracks the Proxy wrappers alongside the raw terminal instances
Plugin-created terminals are now routed through the terminal manager when terminal.grouping.mode is set to tree, instead of always appearing as separate tabs in the bottom panel

How to test

Terminal identity fix (ms-python):

Open a Python file in the example application
Click "Run Python File" in the editor toolbar
Close the terminal that was opened
Click "Run Python File" again
Verify the terminal opens and runs again (previously it became unresponsive)

Terminal manager tree mode with plugin-created terminals:

Set terminal.grouping.mode to tree in preferences
Click "Run Python File" (or any extension that creates a terminal)
Verify the terminal appears inside the terminal manager tree, not as a separate tab in the bottom panel
Close and re-run to verify it continues working

Task terminals in tree mode:

With terminal.grouping.mode set to tree, run a task (e.g. via Terminal > Run Task)
Verify the task terminal appears on the Tasks page in the terminal manager (not as a regular page)
Verify the task completes normally

Integrated terminal in tree mode:

With terminal.grouping.mode set to tree, create a new terminal via the command palette or Ctrl+`
Verify it appears in the terminal manager tree
Split the terminal and verify the split also appears correctly in the tree
Verify typing and command execution works normally

Tabbed mode (regression check):

Switch terminal.grouping.mode to tabbed
Create a new terminal, run a task, and run a Python file
Verify all terminals appear as separate tabs in the bottom panel as before

Switching modes at runtime:

Start with tabbed mode and create some terminals
Switch to tree mode and verify existing terminals migrate to the manager
Switch back to tabbed and verify terminals migrate back to tabs

Example python file to test:

from datetime import datetime

def main():
    now = datetime.now().strftime("%Y-%m-%d %H:%M:%S")
    print(f"The current time is: {now}")
    date = datetime.now().strftime("%Y-%m-%d")
    print(f"The current date is: {date}")
    weekday = datetime.now().strftime("%A")
    print(f"The current weekday is: {weekday}")

if __name__ == "__main__":
    main()

Follow-ups

The editor/title/run toolbar button uses a hardcoded debug-alt icon instead of dynamically picking up the primary command's icon from the contributing
extension
Localization keys from bundled sub-extensions (e.g. %debugpy.command.debuginterminal.title%) may appear unresolved in the toolbar dropdown

Breaking changes

This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines
User-facing text is internationalized using the nls service (for details, please see the Internationalization/Localization section in the Coding Guidelines)

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

eneufeld

Looks good to me overall. Some small things to check though.

eneufeld · 2026-04-13T21:07:54Z

            const parentTerminal = options.location.parentTerminal;
            if (parentTerminal instanceof TerminalExtImpl) {
                for (const [k, v] of this._terminals) {
                    if (v === parentTerminal) {


could you check whether this path works correctly? parentTerminal is a proxy (from createAPIObject) but v is the raw object see the obtainTerminal method. This might have been there earlier but we should fix this.

Good catch! You're right, the === comparison would fail when a plugin passes the API proxy object (which is what plugins receive) since _terminals only stores the raw TerminalExtImpl. Fixed by checking _terminalApiObjects first, then falling back to _terminals.

eneufeld · 2026-04-13T21:10:58Z

            this._pseudoTerminals.set(id, new PseudoTerminal(id, this.proxy, options.pty));
            return this.proxy.$createTerminal(id, { name: options.name }, undefined, true);
        } else {
            return this.proxy.$createTerminal(id, profile.options);


do we need to store an instance in the _terminalApiObject map?

No, this path is fine as-is. $startProfile is an RPC call from the Main side, not the createTerminal path that plugins use directly. It calls this.proxy.$createTerminal(), which later triggers $terminalCreated on the Ext side via RPC, populating _terminals through obtainTerminal. Since no apiObjectWrapper is involved (there is no plugin caller receiving the return value), the raw TerminalExtImpl in _terminals is the API object, and the getApiObject fallback handles it correctly.

eneufeld · 2026-04-13T21:13:05Z

-    get terminals(): TerminalExtImpl[] {
-        return [...this._terminals.values()];
+    get terminals(): theia.Terminal[] {
+        return [...this._terminals.keys()].map(id => this.getApiObject(id)!).filter(t => t !== undefined);


why the ! after getApiObject? we filter anyway don't we?

You're right, the ! was unnecessary since the .filter() already handles undefined. Removed the ! and added a type predicate to the filter instead: .filter((t): t is theia.Terminal => t !== undefined).

eneufeld · 2026-04-13T21:14:37Z

should we add tests for this file? what and when is returning the correct instance is quite confusing

eneufeld · 2026-04-13T21:17:03Z

this is unused now

eneufeld · 2026-04-13T21:17:41Z

    protected registerTerminalListener(): void {
        this.commandHandlerDisposables.push(
            this.terminalFrontendContribution.onDidCreateTerminal(async terminal => {
-                if (this.isDebugTerminal(terminal)) {


so we now support all terminals and only ignore the ones for tasks? why this change?

The old code only handled debug terminals (routing them into the manager). The new listener routes all externally-created terminals except tasks and hidden ones. This broadening is intentional: plugin-created terminals have no special kind property, so filtering by kind would not catch them. Task terminals are excluded because they have their own dedicated "Tasks" page (see #17105). Hidden terminals are excluded by design. Debug terminals and regular terminals should both appear in the tree manager when created externally.

eneufeld · 2026-04-13T21:20:20Z

+     * Set to `true` while {@link createTerminalWidget} is executing, so that external
+     * listeners can distinguish internally created terminals from external ones (e.g. from plugins).
+     */
+    creatingTerminalInternally = false;


if createTerminalWidget ever is called concurrently, this simple boolean flag will fail. we could use a counter instead. not sure this is worth doing .

Replaced with a counter. It's a small change and makes the code correct under concurrent calls. The getter still returns a boolean so the external API is unchanged.

Resolves GH-16570 - Terminal events now fire with the same Proxy object returned to extensions - This fixes strict equality checks (===) failing in extensions like ms-python - Added apiObjectWrapper parameter to createTerminal for internal registration - Updated terminals getter and activeTerminal to return API objects - Updated $provideTerminalLinks to pass API object to link providers

…nager Part of GH-16570 - Extend terminal creation listener to intercept externally created terminals (e.g. from plugins) and add them to the terminal manager when grouping mode is set to 'tree' - Add creatingTerminalInternally flag to TerminalManagerWidget to distinguish internally vs externally created terminals - Move registerTerminalListener into registerHandlers so it is consistently (un)registered with preference changes

- Remove unnecessary non-null assertion in terminals getter - Fix parentTerminal lookup to check API proxy objects, not just raw terminals - Remove unused isDebugTerminal method - Use counter instead of boolean for creatingTerminalInternally flag - Add terminal-ext.spec.ts with 22 tests for API object identity and events Contributes to GH-16570

eneufeld

LGTM, thank you

github-project-automation bot added this to PR Backlog Apr 10, 2026

github-project-automation bot moved this to Waiting on reviewers in PR Backlog Apr 10, 2026

ndoschek requested a review from eneufeld April 10, 2026 14:12

eneufeld requested changes Apr 13, 2026

View reviewed changes

github-project-automation bot moved this from Waiting on reviewers to Waiting on author in PR Backlog Apr 13, 2026

ndoschek added 3 commits April 14, 2026 13:35

ndoschek requested a review from eneufeld April 14, 2026 16:13

ndoschek force-pushed the GH-16570 branch from 26060c6 to 0b104a0 Compare April 14, 2026 16:13

eneufeld approved these changes Apr 16, 2026

View reviewed changes

github-project-automation bot moved this from Waiting on author to Needs merge in PR Backlog Apr 16, 2026

ndoschek merged commit e960af2 into master Apr 17, 2026
10 checks passed

ndoschek deleted the GH-16570 branch April 17, 2026 12:16

github-project-automation bot moved this from Needs merge to Done in PR Backlog Apr 17, 2026

github-actions bot added this to the 1.71.0 milestone Apr 17, 2026

Conversation

ndoschek commented Apr 10, 2026

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

Uh oh!

eneufeld left a comment

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!

eneufeld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants