feat: summary status bar and improve process handling #114

justfalter · 2025-11-12T23:52:02Z

This PR started with a desire to add a summary status bar, but I quickly found some improvements were required to command handling to make that happen.

Improves visibility into the state of the extension by adding a Run On Save Status status bar. Text of running Sync: 3 Async: 2 indicates that the extension is currently running 3 synchronous and 2 async commands. Text of idle Sync: 0 Async: 0 indicates that nothing is currently running.
- Clicking the status bar reveals the Run On Save output.
Removes vestigial status bar code that stopped working a long time ago.
Improves synchronous command handling:
- Sync commands are processed one at a time in FIFO order. Prior to this change, saving 20 files at the same time (like with a search/replace) would lead to 20 separate sync commands being run simultaneously. With this change, the 20 commands would be executed one at a time.
When disabled, terminates all running commands. This provides a much-needed means to terminate commands when things have gone awry.
- The signal used to kill the processes is controlled via command's killSignal property, defaulting to SIGTERM.

Description of Changes

Summarize changes this PR introduces.

Testing

Poke around with the status bar.

Have run on save config, verify that the status bar displays expected information.

Verify disabling extension terminates running commands.

The following command will spawn sync and async commands, each starting a "sleep":

{
  "emeraldwalk.runonsave": {
    "commands": [
      {
        "match": ".*",
        "cmd": "sleep 99999"
      },
      {
        "match": ".*",
        "cmd": "sleep 88888",
        "isAsync": true
      }
    ]
  }
}

Save several files
Observe that the number of sync and async commands have counted upward.
run ps aux| grep sleep and observe the accumulation of a sleep 99999 sync command and several sleep 88888 async commands.
Run the Run On Save: Disable command.
Observe that the processes have been terminated and the queue cleared.

Checklist

Did you update any relevant docs / samples?
Did you include details on how to test your changes?

This PR started with a desire to add a summary status bar, but I quickly found some improvements were required to command handling to make that happen. - Improves visibility into the state of the extension by adding a `Run On Save Status` status bar. Text of `running Sync: 3 Async: 2` indicates that the extension is currently running 3 synchronous and 2 async commands. Text of `idle Sync: 0 Async: 0` indicates that nothing is currently running. - Clicking the status bar reveals the Run On Save output. - Removes vestigial status bar code that stopped working a long time ago. - Improves synchronous command handling: - Sync commands are processed one at a time in FIFO order. Prior to this change, saving 20 files at the same time (like with a search/replace) would lead to 20 separate sync commands being run simultaneously. With this change, the 20 commands would be executed one at a time. - When disabled, terminates all running commands. This provides a much-needed means to terminate commands when things have gone awry. - The signal used to kill the processes is controlled via command's `killSignal` property, defaulting to `SIGTERM`.

justfalter · 2025-11-12T23:54:13Z

@bmingles My team makes heavy use of the run on save extension, and I'd really appreciate your feedback! Thank you very much!

bmingles · 2025-11-20T16:51:46Z

@justfalter Sorry for the delay. Lots of competing priorities. I'll try to take a look at the changes sometime this week when I can properly test.

justfalter · 2025-11-20T16:55:38Z

@justfalter Sorry for the delay. Lots of competing priorities. I'll try to take a look at the changes sometime this week when I can properly test.

Thank you, @bmingles! I appreciate any time that you can give!

bmingles

@justfalter I've read through the diff and left a few comments.

I still need to actually test and make sure I fully understand the new queueing mechanism.

I also specifically want to test in Windows to make sure there are no surprises with the kill signal configurations since not all are supported by Windows.

Thanks again for the PR. I'm glad to have real users fixing real problems they are experiencing.

README.md

bmingles · 2025-11-22T03:40:35Z

src/extension.ts

 }

-class RunOnSaveExtension {
+interface runCommandConfig {


This interface should go in the model.ts file. Also, minor thing, but convention for types / interfaces is PascalCase

src/extension.ts

bmingles · 2025-11-22T04:28:25Z

src/extension.ts

+    });
+  }
+
+  public disable(): void {


I'd also make this one async

bmingles · 2025-11-22T04:59:14Z

package.json

                      "error"
                    ]
+                  },
+                  "killSignal": {


I need to test on Windows and make sure there aren't any footguns here since not all kill signals are supported there.

Node's documentation for kill makes no mention of what it does for Windows platforms, but a look at Node's source code reveals that it will unconditionally terminate a process when SIGQUIT, SIGTERM, SIGINT, or SIGKILL are provided. Attempts to send any other signal appears to result in an error.

bmingles · 2025-11-26T03:28:31Z

src/extension.ts

+    const atLeastOneCommandIsRunning = unfinishedSyncCount !== 0 || asyncCount !== 0;
+
+    if (!this.isEnabled()) {
+      // If we are disabled but have unfinished commands, then we are "draining".


What's the scenario that can result in "draining"? Since commands are killed when extension is disabled, is there any meaningful time that we would see this status?

The draining state simply reflects the knowledge that it can take time for a process to fully exit after receiving a termination signal. It's entirely possible that a process ignores a SIGTERM, so the draining state would allow the user to be aware of that instance.

Ah makes sense. In that case, I'm good with the state, but I think I'd like to name it something a little more generic like "stopping" since I think "draining" may not be as broadly recognized.

src/extension.ts

bmingles

@justfalter First of all, thank you for your contribution. I appreciate the help in improving the extension.

Inline Comments

I've done some testing and reviewed the changes a bit closer. I've left a handful of inline comments in the code.

Command Management Changes

Regarding the command management changes, there are 2 primary concerns I have related to changes in existing functionality.

Parallel commands (isAsync:true) that are configured after sequential ones (isAsync:false) used to wait until the preceeding sequential command completed. It seems that the parallel commands execute immediately now. They should wait for prior sequential commands to finish. For reference, I used this configuration to test:

"emeraldwalk.runonsave": {
      // Messages to show before & after all commands
      "message": "*** All Start ***",
      "messageAfter": "*** All Complete ***",
      // Show elappsed time for all commands
      "showElapsed": true,
      "commands": [
        {
          // This cmd should block all following commands (sequential and parallel ones)
          "match": ".*",
          "cmd": "sleep 3",
          "message": "1. sleep 3"
        },
        {
          // This cmd should wait until the first finishes
          "match": ".*",
          "cmd": "sleep 10",
          "message": "2. sleep 10",
          "isAsync": true
        },
        {
          "match": ".*",
          "cmd": "sleep 4",
          "message": "3. sleep 4",
          "isAsync": true
        }
}

I think your observation that saving multiple files can cause many exec commands to fire at once is a good one and something I did not account for. My concern with the current changes is that it may impact existing users that are expecting the pre-existing behavior. For example, a user may actually want 20 exec commands to fire off in parallel across multiple files. Changing them to run sequentially across files could introduce unexpected lag in certain cases. I think it's a good fix, but it should be opt-in via a setting of some sort.

This does introduce some additional complexity since the command queue triggered by each file save will need to honor the rule mentioned in bullet 1 but also be able to order sequential commands across files if a user opts in to this feature.

Thanks again for the PR. Let me know if you need any clarification on my comments.

Co-authored-by: Brian Ingles <github@emeraldwalk.com>

justfalter · 2025-11-26T21:47:06Z

@justfalter First of all, thank you for your contribution. I appreciate the help in improving the extension.

Inline Comments

I've done some testing and reviewed the changes a bit closer. I've left a handful of inline comments in the code.

Command Management Changes

Regarding the command management changes, there are 2 primary concerns I have related to changes in existing functionality.

Parallel commands (isAsync:true) that are configured after sequential ones (isAsync:false) used to wait until the preceeding sequential command completed. It seems that the parallel commands execute immediately now. They should wait for prior sequential commands to finish. For reference, I used this configuration to test:

You're completely right -- I'm not sure how/why I came to the conclusion that the async commands would fire immediately.
I'll work on rectifying this in my PR.

I think your observation that saving multiple files can cause many exec commands to fire at once is a good one and something I did not account for. My concern with the current changes is that it may impact existing users that are expecting the pre-existing behavior. For example, a user may actually want 20 exec commands to fire off in parallel across multiple files. Changing them to run sequentially across files could introduce unexpected lag in certain cases. I think it's a good fix, but it should be opt-in via a setting of some sort.

This does introduce some additional complexity since the command queue triggered by each file save will need to honor the rule mentioned in bullet 1 but also be able to order sequential commands across files if a user opts in to this feature.

A global option to control the behavior would be fair. The only difference between the two modes would be the use of a single queue vs one per save event... I'll probably have to extract the queue out of the extension class...

Thanks again for the PR. Let me know if you need any clarification on my comments.

Gladly. Everything you're saying so far makes sense to me!

- Match prior behavior of only launching async commands when encountered in config order. We do not wait for an async command to complete before starting the next command.

bmingles · 2025-12-15T01:17:26Z

@justfalter FYI, I merged a couple PRs that will require a rebase or merge down

prettier is now installed and checked as part of PRs to main. This is likely to be the source of most of the branch conflicts. You might be able to make this easier by running npx prettier src --write before attempting to rebase / merge down, but I'm not sure.
there are now some basic tests that verify the sequencing of commands npm test. This will hopefully make it a little bit easier to confirm the changes you make don't change existing sequencing

bmingles requested changes Nov 22, 2025

View reviewed changes

bmingles reviewed Nov 22, 2025

View reviewed changes

bmingles reviewed Nov 26, 2025

View reviewed changes

src/extension.ts Outdated Show resolved Hide resolved

bmingles requested changes Nov 26, 2025

View reviewed changes

justfalter and others added 3 commits November 26, 2025 14:28

move RunCommandConfig to model.ts

6215bb0

make enable/disable async

5a5f45b

Apply suggestions from code review

5573b81

Co-authored-by: Brian Ingles <github@emeraldwalk.com>

restore processing of async commands in config order

83a7b4a

- Match prior behavior of only launching async commands when encountered in config order. We do not wait for an async command to complete before starting the next command.

bmingles mentioned this pull request Nov 28, 2025

Display a message to the user: status bar or notification? #39

Open

feat: summary status bar and improve process handling #114

Are you sure you want to change the base?

feat: summary status bar and improve process handling #114

Uh oh!

Conversation

justfalter commented Nov 12, 2025

Description of Changes

Testing

Poke around with the status bar.

Verify disabling extension terminates running commands.

Checklist

Uh oh!

justfalter commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmingles commented Nov 20, 2025

Uh oh!

justfalter commented Nov 20, 2025

Uh oh!

bmingles left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmingles Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmingles Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

bmingles Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

justfalter Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

bmingles Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

justfalter Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

bmingles Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmingles left a comment

Choose a reason for hiding this comment

Inline Comments

Command Management Changes

Uh oh!

justfalter commented Nov 26, 2025

Inline Comments

Command Management Changes

Uh oh!

bmingles commented Dec 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

justfalter commented Nov 12, 2025 •

edited

Loading