Skip to content

Conversation

@shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Aug 9, 2023

The getCurrentQuery method determines the "active" query by checking the VS Code activeTextEditor. Due to how VS Code defines text editors, this leads to weird cases where it can consider comment/discussion boxes as the active text editor, instead of the underlying file 😅 E.g. we have a bug in the CodeQL code tour, where we accidentally set this comment as the active editor.

🔗 See internal linked issue for more details!

image

To avoid this, we can look at the active editor tab instead. This doesn't change the existing behaviour, and fixes the weird comment/discussion box bug. I've tested this in various cases, including running quick eval, quick queries, and just running queries from various places (context menu, file explorer, command palette) 😄

Note that there are some other places in the code where we use window.activeTextEditor. We could change all of those occurrences, but I'm hesitant to overcomplicate things where it's probably not necessary 🙈

👀

@dbartol, would you mind giving this a quick look/test? The bug first occured after 0bd0bf1 and the getCurrentQuery method is also used in the new debugger here. I want to be sure that the "save dirty documents" and debugging stuff all still works as expected too!

Checklist

I'm not sure this needs a changelog entry, since it doesn't change any behaviour? 🤔 (and the codespace template/code tour is still in beta)

  • 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.

@shati-patel shati-patel marked this pull request as ready for review August 9, 2023 16:38
@shati-patel shati-patel requested a review from a team as a code owner August 9, 2023 16:38
@adityasharad
Copy link
Contributor

This likely explains many silent failures I saw while attempting query-running operations in the CodeTour that I could not reliably reproduce or understand!

@shati-patel shati-patel requested a review from dbartol August 9, 2023 16:41
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I did help to write it

Still likely worth getting a 👍🏼 from @dbartol on the if possible to make sure it will works with the debugging UI

@dbartol
Copy link
Contributor

dbartol commented Aug 14, 2023

This shouldn't affect query debugging (other than the intended effect, of course). All the "save dirty documents" code just calls workspace.saveAll(), without attempting to determine the current editor/tab on its own.

@dbartol dbartol merged commit e308c2b into main Aug 14, 2023
@dbartol dbartol deleted the shati-patel/get-current-query branch August 14, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants