feat(stepfunctions): Prevent launching Workflow Studio with invalid JSON + save metric refactoring

[witness-me]

Problem

1. Currently, the Workflow Studio (WFS) webview opens even when the JSON definition is invalid, which causes errors in the webview since WFS requires an initial, valid JSON definition.
2. The current metric for saving the file lacks a clear name and is not optimally defined for tracking save/sync events.
3. There is an issue with the undo functionality (CTRL+Z) inside WFS where the webview definition does not reflect changes in the local file. This occurs because VSCode's built-in undo command interferes with the WFS undo.
4. The production CDN link is missing in the code (since it was not set up before)

Solution

1. Detect invalid JSON files and prevent WFS from opening if JSON is invalid. In such cases, switch to the default editor and display a warning message.
2. Refactor and rename the save/sync metric for clarity and modify its parameters for improved tracking.
3. Suppress VSCode's built-in undo command when WFS webview is focused to prevent conflicts, allowing WFS to manage undo operations directly.
4. Production CDN link is added

---

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

[github-actions]

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

[witness-me]

Demo recording

[jpinkney-aws]

is there any reason we can't use camel case here?

[witness-me]

I used it this way because the previous version of the metric that I was using (saveType introduced by Threat Composer integration) had the same screaming snake casing.

I can update the metric to camel case to keep this consistent with most of the types

[jpinkney-aws]

Can you explain more about why this is needed?

[witness-me]

Sure thing!

In VSCode, the Workflow Studio integration works as a custom editor, so it inherits all the standard VSCode keybindings, including Ctrl+Z for undo. At the same time, WFS has its own undo keybinding (Ctrl+Z) to handle undo actions specifically within the WFS context, like those in the console.

However, these actions conflict with each other in the IDE. When the user presses Ctrl+Z, both WFS and the IDE try to perform undo actions simultaneously. This leads to a bug in the established logic: undoing via hotkeys in WFS applies the action within WFS but doesn’t properly reflect the changes in the local file, unlike regular edits or when undo is triggered using the WFS interface button.

The solution is to suppress VSCode’s default undo behavior when the WFS integration is active and focused, to prevent this interference. I tried to avoid adding new custom state for tracking and instead relied on context keys, but unfortunately, context keys don’t seem to be available for custom editors.

[jpinkney-aws]

Makes sense. Is there any way to intercept the save command on the custom editor using something like

webView.webview.onDidRecieveMessage(e => {
// whatever save does here
})

[witness-me]

I belive you mean undo in your comment, rather than save.

The command is executed inside WFS and only affects WFS internal definition, the webview does not receive any message for it.
With the suppression logic I introduced, it should work like this:

1. User triggers undo in WFS (from their keyboard or the button in the UI)
2. WFS performs its internal undo and updates its internal definition to the previous one
3. Updated WFS internal definition is synced with the local file
4. At the same time, VSCode built-in undo is suppressed for Ctrl+Z to not interfere with the above behaviour

I did not find any better suppression options since the custom editor (including webview) has all the standard VSCode keybindings working (such as Ctrl+Z) unless we override them

[jpinkney-aws]

Ah interesting, I think that's fine then

[jpinkney-aws]

Let me check with my team to see if they have any ideas how to do this without a noop command

[justinmk3]

why are you renaming this field? fields should not be over-specific like this

[witness-me]

saveType is a field name introduced by threat composer, which only allows 2 values: "MANUAL_SAVE" | "AUTO_SAVE".

For our use case, we want to add auto-sync option, as well track where the action originated (either in WFS or VSCode). If we have the latter as a separate metric, it will not be clear which specific action originated where (since we have both save and sync actions).

Do you belive there is a better way to achieve that?

[justinmk3]

saveType is a field name introduced by threat composer, which only allows 2 values: "MANUAL_SAVE" | "AUTO_SAVE".

add more values. enums are just lists that guide string values.

[witness-me]

I can add an action such as AUTO_SYNC in that list, I just wanted to make sure that can track the metric with enough granularity in that case. We have 3 scenarios:

• User saves from VSCode
• User saves from custom editor by clicking on the button in the UI
• User makes the change in custom editor and this change is auto-synced with local file
  We want to catch a usage for each of these scenarios, I'm not sure if we can achieve that with separate metrics.

The way you are suggesting we would need to post 2 metrics for save type such as

span.record({
  saveType: 'MANUAL_SAVE',
  source: 'VSCODE',
})

Can you confirm if we will be able to query a metric by multiple fields when constructing a dashboard? I.e. we'll want to detect detect save events originating from custom editor vs VSCode (and exclude AUTO_SYNC event)

[justinmk3]

The way you are suggesting we would need to post 2 metrics for save type such as

span.record({
  saveType: 'MANUAL_SAVE',
  source: 'VSCODE',
})

By "2 metrics" do you mean "2 fields in 1 metric" (which aligns with the code example)? That's normal and idiomatic, and no problem for dashboards.

[justinmk3]

all of these stepfunctions metrics need to be upstreamed to https://github.com/aws/aws-toolkit-common

they have been in this override file for a long time

[witness-me]

It have already been raised in one of the previous pull requests, we are still developing the feature and we will most probably need to add more metrics, which is more handy to do in a single package.

This approach also aligns with what is written in telemetry readme:

When your feature is released, the "development" metrics you defined in vscodeTelemetry.json should be upstreamed to aws-toolkit-common.

We have a SIM in our project to transfer metrics to that package closer to launch. Can we keep them in the current package for now?

[justinmk3]

Can we keep them in the current package for now?

What is the ECD?

[witness-me]

we are planning to launch around end of February

[justinmk3]

please read https://github.com/aws/aws-toolkit-vscode/blob/master/docs/telemetry.md

this looks like a failure mode. throw an error from your code, and then telemetry.xx.run() will correctly set result=Failed and reason=<error code>. Don't add a highly-specific boolean to signal a failure.

[witness-me]

This is not a failure, but rather an expected case for which we are syncing webview content with the file as usual. This variable is just to track this case in telemetry, not to handle this differently

[justinmk3]

How is invalid json not a failure?

[witness-me]

Since we are launching our custom editor by default, we want to sync the webview content with local file as much as we can. If the user opens code editor inside our integration and types something making JSON invalid, we still want to sync that invalid JSON to the local file as a string to prevent user from losing any data (e.g. if they close VSCode)

[justinmk3]

this is an extremely verbose way to do a very simple, common thing.

[witness-me]

I can rewrite it into two more simple functions like this

export const isInvalidJsonFile = (textDocument: vscode.TextDocument): boolean => {
    const documentContent = textDocument.getText().trim()
    // An empty file or whitespace-only text is considered valid JSON for our use case
    return textDocument.fileName.toLowerCase().endsWith('.json') && documentContent
        ? isInvalidJson(documentContent)
        : false
}

const isInvalidJson = (content: string): boolean => {
    try {
        JSON.parse(content)
        return false
    } catch {
        return true
    }
}

I hope this will work, otherwise please let me know if I need to consider something else

[justinmk3]

run() will automatically set reason to this code. Thus there is no need for the isInvalidJson field.

[witness-me]

This code does not have anything to do with isInvalidJson metric, which indicates that the JSON code inside the custom webview is invalid. Meanwhile, this code is responsible for detecting invalid JSON code in local file to prevent opening webview integration in the first place.