Skip to content

CodeQL model editor: Show access path suggestions in the webview#3305

Merged
shati-patel merged 4 commits intomainfrom
shati-patel/display-suggestions
Feb 5, 2024
Merged

CodeQL model editor: Show access path suggestions in the webview#3305
shati-patel merged 4 commits intomainfrom
shati-patel/display-suggestions

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel commented Feb 1, 2024

Reads the access path suggestions (from #3304 and previous PRs) in the webview and shows them in a "suggest box":

image

This is only enabled for Ruby, so the public model editor (i.e. for Java & C#) is unaffected. See internal linked issue for more details 🔗

❓ Should this have a view test? The SuggestBox component is already tested, so I wasn't sure if it made sense to test e.g. the ModelInputSuggestBox separately too.

Checklist

N/A—still feature-flagged for internal use 🦞

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


const Container = styled.div`
width: 430px;
display: flex;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was a styling fix to stop the model editor looking like this 😅

image

@shati-patel shati-patel marked this pull request as ready for review February 1, 2024 10:23
@shati-patel shati-patel requested review from a team as code owners February 1, 2024 10:23
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Would we already expect the icon and details to be visible in the suggestions as well? It currently seems to be missing (probably because we're not passing getIcon and getDetails).

Screenshot 2024-02-01 at 14 02 42

Also, it seems like for some type models, we're not able to edit anything, while for others, we are. This might be unrelated to your changes.

Screenshot 2024-02-01 at 14 01 23

I think we can also use the suggest box for the type path (i.e. the text field shown in the input field for type models), but this would need to use the output suggestions (since the ReturnValue is valid for type models). This can also be done in a follow-up PR.

Comment thread extensions/ql-vscode/src/view/model-editor/ModelInputSuggestBox.tsx Outdated
@shati-patel
Copy link
Copy Markdown
Contributor Author

Would we already expect the icon and details to be visible in the suggestions as well? It currently seems to be missing (probably because we're not passing getIcon and getDetails).

Woops, I definitely just forgot to add them 😅 Done now in the latest commit:
image

(I stole the styling for the icon from the storybook example)

Also, it seems like for some type models, we're not able to edit anything, while for others, we are. This might be unrelated to your changes.

I think we can also use the suggest box for the type path (i.e. the text field shown in the input field for type models), but this would need to use the output suggestions (since the ReturnValue is valid for type models). This can also be done in a follow-up PR.

Hmm yes 🤔 I can take a look at the type paths etc in a separate PR!

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@shati-patel shati-patel merged commit 7c233db into main Feb 5, 2024
@shati-patel shati-patel deleted the shati-patel/display-suggestions branch February 5, 2024 14:16
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.

2 participants