Fix loading results in second opened view

[koesie10]

When results were already cached in memory and the view requested the result, it would not be loaded because the event would not be fired. This fires the event when the result is loaded from cache as well, to ensure that the view always receives the result.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[elenatanasoiu]

Do we need to do this in the next if statement as well?

[koesie10]

We shouldn't need to do that, that should already be handled in the loadResultsIntoMemory method:

vscode-codeql/extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts

Line 157 in bc65d8f

this._onResultLoaded.fire(result);

[elenatanasoiu]

Yup, but the next if statement is actually calling loadResultsFromStorage instead of loadResultsIntoMemory:

if (options?.skipCacheStore) {
  return this.loadResultsFromStorage(
    variantAnalysisId,
    variantAnalysisStoragePath,
    repositoryFullName,
  );
}

[koesie10]

That's true, but that if-statement is explicitly created to skip storing the results anywhere, which should include not firing the event. If we did fire the event in there, it would distribute the result to the view, which would then store it in memory. By setting the skipCacheStore option, the caller is explicitly saying that they don't want the result to be stored anywhere.

This option is used when exporting results. For example, if there are 1,000 results, you might want to export all 1,000 results to a file. We need to load the SARIF files into memory to create the Markdown files, but this is very temporary. Let's say the SARIF file results in a memory usage of 30MB. Then, we'd use 30MB while exporting, and return to the previous memory usage once exporting is done. If this option is not set, we'd use 30,000 MB (30 GB) after exporting since every result is stored in cache.

If we were to now change the behaviour to fire the event, we would still be storing that 30 GB of memory in the view, rather than in the extension host. I don't think this is what we want to do.

[elenatanasoiu]

I love the example! :) Thanks! That makes sense.

[elenatanasoiu]

Would you be up for writing a test for this as well? It would involve listing the three cases:

• result already exists, make sure it's loaded
• result doesn't exist
  • the skipCacheStore option is on: make sure it loads results into memory and does create a cache key
  • the skipCacheStore option is off: make sure it loads results into memory and creates a cache key

[koesie10]

Would you be up for writing a test for this as well? It would involve listing the three cases:

Sure, I should be able to write some simple tests for this. Since the Jest migration is not yet merged into main (since it's still waiting on #1801), I'm a bit hesitant to start on it now since this will probably involve quite a lot of stubbing (we need to either load results from the filesystem or stub the loadResultsFromStorage method).

[elenatanasoiu]

Ah, right. Perhaps a ticket for later then?

[koesie10]

Ah, right. Perhaps a ticket for later then?

I've created a ticket for this, see the internal linked issue.