Conversation
This adds a new filtering on SARIF code snippets for very large code snippets (defined as 8MB or more). If less than 1% of such a snippet is highlighted, it will not include the code snippet in the analysed results, and it will thus not be shown in the UI. This is to avoid very large SARIF files that can cause the extension host to crash when the analysis results are send to the UI. I don't think any of these snippets would ever be useful to show, so it should be fine to just not include them.
|
I'm not sure if this is the best approach to make the UI work in more cases, but this makes the results view work for vercel/next.js (it still takes more than 10 seconds to load, but that can be fixed in a follow-up PR). If you think it's useful, I'd be happy to add tests for this behaviour as well. |
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Thanks for implementing this. I agree with the logic about region sizes and this should hopefully catch cases of generated or minimized code. I think we can certainly go with this as a first attempt and adjust the logic if we find other cases we want to filter out, so I'm not too worried about trying to make the logic absolutely perfect in this PR.
I have a few comments about variable naming, but the actual logic in the code looks correct to me 👍
|
Sorry just saw your other comment. Tests for this would be great too! |
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Thanks for the updates and tests
2 participants
This adds a new filtering on SARIF code snippets for very large code snippets (defined as 8KB or more). If less than 1% of such a snippet is highlighted, it will not include the code snippet in the analysed results, and it will thus not be shown in the UI.
This is to avoid very large SARIF files that can cause the extension host to crash when the analysis results are send to the UI. I don't think any of these snippets would ever be useful to show, so it should be fine to just not include them.
Checklist
ready-for-doc-reviewlabel there.