Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote queries: Open query file/text from webview #1041

Merged
merged 5 commits into from
Dec 14, 2021

Conversation

shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Dec 8, 2021

Adds some interaction to these buttons in the remote queries webview:

image

  • The first one opens the query file that was run, if it exists (similar to what we do in the local results view).
  • The second one opens the exact query text that was run (in a virtual "read-only" file).

Follow up

Checklist

N/A: Still an internal-only feature. No user-visible changes.

  • 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.
  • @github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@charisk charisk deleted the branch github:main December 9, 2021 10:05
@charisk charisk closed this Dec 9, 2021
@shati-patel shati-patel reopened this Dec 9, 2021
@shati-patel shati-patel changed the base branch from remote-query-monitor-and-render-results to main December 9, 2021 10:22
@shati-patel
Copy link
Contributor Author

Latest commit adds a temporary file containing exact query text.

In the local version of showing query text, we create a "text provider" and write to a read-only document:

// displays query text in a read-only document
vscode.workspace.registerTextDocumentContentProvider('codeql', {
provideTextDocumentContent(
uri: vscode.Uri
): vscode.ProviderResult<string> {
const params = new URLSearchParams(uri.query);
return (
(JSON.parse(params.get('isQuickEval') || '')
? SHOW_QUERY_TEXT_QUICK_EVAL_MSG
: SHOW_QUERY_TEXT_MSG) + params.get('queryText')
);
},
});

I couldn't work out how to re-use that code, or just import the relevant parts (creating a read-only doc), so I've gone with a more lightweight approach of writing to a temporary file for now. Happy to adjust if anyone has suggestions! 🙇🏽‍♀️

@shati-patel shati-patel marked this pull request as ready for review December 9, 2021 16:42
@shati-patel shati-patel requested a review from a team as a code owner December 9, 2021 16:42
// Get the query text from query file and save it in a temporary .ql file.
const queryTextTmpFilePath = path.join(tmpDir.name, `tmp-${queryName}`);
const queryText = await fs.readFile(queryFilePath, 'utf8');
await fs.writeFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this creates the temp file before a user ever tries to open it (and the user may never try to do so). Also, this file may get cleaned up if the user restarts vscode. Can we ever get a situation where the file is removed before someone tries to open it (eg- after a restart)?

Maybe it's safer to generate this file only when requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a different approach (using a virtual file), so this temporary file no longer exists! ☁️

(There's still the general problem that remote query data doesn't persist across restarts, but that's beyond this PR 😅 We have a separate issue for that somewhere)

@charisk
Copy link
Contributor

charisk commented Dec 10, 2021

In the local version of showing query text, we create a "text provider" and write to a read-only document

Do you want to have a look at that together?

@shati-patel
Copy link
Contributor Author

In the local version of showing query text, we create a "text provider" and write to a read-only document

Do you want to have a look at that together?

We worked through this using the existing query-history.ts example and the VS Code virtual doc sample as a guide 🐄 🎉

I've registered a new "remote-query" text provider to let us write to a readonly file. Ideally, I'd like to slightly refactor the QueryHistoryManager code, so that we use a single codeql text provider everywhere (instead of registering two separate ones). To avoid this PR getting too big, I'm happy to do that in a separate PR, unless there's a more urgent need to do it all together here!

Copy link
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Looks great! Only super minor things.

extensions/ql-vscode/src/extension.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Lets make sure the styling is correct for the new buttons.

private async openFile(filePath: string) {
try {
const textDocument = await workspace.openTextDocument(filePath);
await Window.showTextDocument(textDocument, ViewColumn.One);
Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually, it would be nice to do something like #1037, but that can happen later. Once #1037 gets in, we can work on extracting the behaviour so it is used wherever we are opening editors.

true
);
const doc = await workspace.openTextDocument(uri);
await Window.showTextDocument(doc, { preview: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this read-only? It's possible to make happen, but I forget how.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it's read-only! That's what the remote-query prefix and the corresponding provideTextDocumentContent function do 😃

@shati-patel shati-patel merged commit 72ff828 into github:main Dec 14, 2021
@shati-patel shati-patel deleted the remote-query-buttons branch December 14, 2021 12:13
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.

3 participants