Address terminal review by cpuguy83 · Pull Request #52 · project-dalec/dalec-vscode-extension

cpuguy83 · 2026-01-16T19:49:31Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

- Guard reuse against closed terminals - Make terminal names authoritative in getOrCreateTerminal - Cache cleanup disposable and add name helper docs - Adjust terminal helper tests for new API Signed-off-by: Brian Goff <cpuguy83@gmail.com>

- Fail early when no workspace root is available - Remove non-null assertions for safer type narrowing - Run terminal cleanup in teardown concurrently Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Copilot

Pull request overview

This PR addresses terminal-related improvements including better terminal cleanup, test reliability enhancements, and API refinements for terminal helper functions.

Changes:

Improved terminal reusability checking by adding exitStatus validation
Enhanced test reliability with unique terminal names and graceful timeout handling
Refactored terminal cleanup disposable management to attempt reuse of disposables

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
src/commands/runBuildCurrentSpecCommand/utils/terminalHelpers.ts	Added `isTerminalReusable` helper, improved terminal registry cleanup, added documentation, changed `getOrCreateTerminal` to use `TerminalOptionsWithoutName` type, and refactored `registerTerminalCleanup` to reuse disposable instances
src/test/terminalHelpers.test.ts	Parallelized terminal disposal in teardown, improved null-safety checks, added unique ID generation for test terminal names, enhanced `waitForTerminalClose` to return boolean status, and removed problematic assertions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-16T19:52:30Z

src/commands/runBuildCurrentSpecCommand/utils/terminalHelpers.ts


 export function registerTerminalCleanup(): vscode.Disposable {
-  if (!terminalCloseSubscription || terminalCloseDisposed) {
+  if (!terminalCloseSubscription || terminalCloseDisposed || !terminalCleanupDisposable) {


The condition logic here is incorrect. This condition will always evaluate to true on the first call because terminalCleanupDisposable starts as undefined.

The condition !terminalCleanupDisposable should be removed because it defeats the purpose of reusing the disposable. The function should only recreate the subscription if it doesn't exist OR if it has been disposed. The correct condition should be:
if (!terminalCloseSubscription || terminalCloseDisposed)

With the current logic, every call to registerTerminalCleanup() will recreate both subscriptions even when they already exist, creating a resource leak where multiple event listeners pile up.

Suggested change

if (!terminalCloseSubscription || terminalCloseDisposed || !terminalCleanupDisposable) {

if (!terminalCloseSubscription || terminalCloseDisposed) {

Copilot · 2026-01-16T19:52:30Z

src/commands/runBuildCurrentSpecCommand/utils/terminalHelpers.ts

+    terminalCleanupDisposable = new vscode.Disposable(() => {
+      if (terminalCloseSubscription) {
+        terminalCloseSubscription.dispose();
+        terminalCloseSubscription = undefined;
+        terminalCloseDisposed = true;
+      }
+    });


This disposal logic is problematic. When the same terminalCleanupDisposable is returned multiple times (line 82), calling dispose() on any of those references will set terminalCloseSubscription to undefined and terminalCloseDisposed to true. However, on the next call to registerTerminalCleanup(), the condition on line 62 will see terminalCloseDisposed === true and recreate everything, including creating a new terminalCleanupDisposable.

This means subsequent calls to dispose() on the old references won't work correctly because they're disposing the old terminalCleanupDisposable which no longer references the current terminalCloseSubscription.

A safer pattern would be to track whether the cleanup disposable itself has been disposed, or to ensure that each call to registerTerminalCleanup() returns a unique disposable that only affects the shared subscription if it's the last reference.

src/test/terminalHelpers.test.ts

Copilot · 2026-01-16T19:52:31Z

src/test/terminalHelpers.test.ts


  test('getOrCreateTerminal matches terminals with numeric suffixes', async () => {
-    const baseName = 'Dalec Build (suffix) - example.yaml';
+    const uniqueId = Date.now().toString(36);


This approach to generate a unique ID may not be sufficiently unique in test scenarios where multiple tests run in parallel or in quick succession. Date.now() returns milliseconds, and .toString(36) creates a relatively short string. If two test runs occur within the same millisecond (or if tests are run in parallel), they could generate the same ID.

Consider using a more robust approach like crypto.randomUUID() or a simple incrementing counter to ensure uniqueness across test runs.

Copilot · 2026-01-16T19:52:31Z

src/test/terminalHelpers.test.ts

  await Promise.race([
    new Promise<void>((resolve) => {
-      const subscription = vscode.window.onDidCloseTerminal((closed) => {
-        if (closed === terminal) {
+      const subscription = vscode.window.onDidCloseTerminal((closedTerminal) => {
+        if (closedTerminal === terminal) {
+          closed = true;
          subscription.dispose();
          resolve();
        }
      });
    }),
-    new Promise<void>((resolve) => setTimeout(resolve, 2000)),
+    new Promise<void>((resolve) => setTimeout(resolve, timeoutMs)),
  ]);


There's a potential race condition here. The closed variable is set inside the promise callback (line 87), but Promise.race resolves as soon as the first promise completes. If the timeout promise resolves first, the function proceeds to line 96 before the closed variable is set to true, even if the terminal actually closes shortly after.

Additionally, the subscription is never disposed if the timeout occurs first, leading to a resource leak. The subscription should be disposed outside the promise or in a finally block to ensure cleanup happens regardless of which promise wins the race.

Copilot · 2026-01-16T19:52:31Z

src/test/terminalHelpers.test.ts

+    await Promise.all(
+      terminals.map(async (terminal) => {
+        terminal.dispose();
+        await waitForTerminalClose(terminal);
+      }),
+    );


The call to waitForTerminalClose in the teardown may not properly handle failures. If waitForTerminalClose returns false (indicating the terminal didn't close), the terminal is still removed from the createdTerminals array (via splice on line 10), but the terminal may still be open. This could lead to lingering terminals between test runs.

Consider tracking which terminals failed to close and either retrying or logging a warning, rather than silently ignoring the issue.

Suggested change

await Promise.all(

terminals.map(async (terminal) => {

terminal.dispose();

await waitForTerminalClose(terminal);

}),

);

const failedToClose: vscode.Terminal[] = [];

await Promise.all(

terminals.map(async (terminal) => {

terminal.dispose();

const closed = await waitForTerminalClose(terminal);

if (!closed) {

failedToClose.push(terminal);

}

}),

);

if (failedToClose.length > 0) {

const names = failedToClose.map((t) => t.name).join(', ');

// Log a warning rather than silently ignoring terminals that failed to close.

console.warn(

`Warning: Failed to close ${failedToClose.length} terminal(s) during teardown: ${names}`,

);

}

cpuguy83 · 2026-01-20T16:56:29Z

I'm actually not comfortable with any of these changes. I need to do some work to understand the API's we are using here.

cpuguy83 added 7 commits January 16, 2026 10:54

Reduce chance of possible race in reusing terminal that is closing

cdff0cb

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Better handle name parameter in getOrCreateTerminal

dc8302b

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Add doc string to getBuildTerminalName

647cee2

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Harden build terminal reuse and cleanup

7e62e1e

- Guard reuse against closed terminals - Make terminal names authoritative in getOrCreateTerminal - Cache cleanup disposable and add name helper docs - Adjust terminal helper tests for new API Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Tighten terminal helper tests

e178972

- Fail early when no workspace root is available - Remove non-null assertions for safer type narrowing - Run terminal cleanup in teardown concurrently Signed-off-by: Brian Goff <cpuguy83@gmail.com>

tests: make terminal names more unique

bee640c

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

tests: treat timeout as a best effort instead of hard failure

2eeb01c

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 requested a review from Copilot January 16, 2026 19:49

Copilot started reviewing on behalf of cpuguy83 January 16, 2026 19:50 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address terminal review#52

Address terminal review#52
cpuguy83 wants to merge 7 commits intomainfrom
address_terminal_review

cpuguy83 commented Jan 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

cpuguy83 commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (!terminalCloseSubscription \|\| terminalCloseDisposed \|\| !terminalCleanupDisposable) {
	if (!terminalCloseSubscription \|\| terminalCloseDisposed) {

-    await Promise.all(
-      terminals.map(async (terminal) => {
-        terminal.dispose();
-        await waitForTerminalClose(terminal);
-      }),
-    );
+    const failedToClose: vscode.Terminal[] = [];
+    await Promise.all(
+      terminals.map(async (terminal) => {
+        terminal.dispose();
+        const closed = await waitForTerminalClose(terminal);
+        if (!closed) {
+          failedToClose.push(terminal);
+        }
+      }),
+    );
+    if (failedToClose.length > 0) {
+      const names = failedToClose.map((t) => t.name).join(', ');
+      // Log a warning rather than silently ignoring terminals that failed to close.
+      console.warn(
+        `Warning: Failed to close ${failedToClose.length} terminal(s) during teardown: ${names}`,
+      );
+    }

Conversation

cpuguy83 commented Jan 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants