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

code monitors: respect default pattern type #63333

Merged
merged 5 commits into from
Jun 26, 2024
Merged

code monitors: respect default pattern type #63333

merged 5 commits into from
Jun 26, 2024

Conversation

stefanhengl
Copy link
Member

@stefanhengl stefanhengl commented Jun 19, 2024

This is part of the Keyword Search GA project (see background below).

The core change is that we use the default pattern type consistently for the query input field and preview. Before, we hardcoded literal as the default and used standard for previews.

This is does not affect existing code monitors.

Other fixes:

  • show suggestions that were previously hidden
  • highlight keyword queries correctly
image

Background:

  • "keyword" will soon be the new default pattern type
  • the default pattern type can be overridden in the user/global settings
  • query fields in all of our products should respect the default

Test Plan:

  • The unit test is currently "skipped" with the following comment
// TODO: these tests trigger an error with CodeMirror, complaining about being
// loaded twice, see https://github.com/uiwjs/react-codemirror/issues/506
  • Manual testing:
    • I created several code monitors with and without pattern type and checked in the DB that the correct pattern type was appended.
    • I configured a new default pattern type in my user settings and verified that the setting changes the default pattern type for code monitors.

Co-authored-by: Felix Kling felix@felix-kling.de

@cla-bot cla-bot bot added the cla-signed label Jun 19, 2024
@github-actions github-actions bot added team/product-platform team/search-platform Issues owned by the search platform team labels Jun 19, 2024
@stefanhengl stefanhengl marked this pull request as ready for review June 19, 2024 09:29
@jtibshirani
Copy link
Member

I also tested the flow where you run a search, then click Actions -> Create monitor. We always carry over the patterntype explicitly, even when it's the default. This seems okay to me, but wanted to highlight it.

Screenshot 2024-06-19 at 9 07 30 AM

@@ -265,10 +268,11 @@ export const FormTriggerArea: React.FunctionComponent<React.PropsWithChildren<Tr
<li>
<ValidQueryChecklistItem
checked={hasValidPatternTypeFilter}
hint="Code monitors support literal and regex search. Searches are literal by default."
hint={`Code monitors support keyword, standard, literal and regex search. The default is ${defaultPatternType}`}
Copy link
Member

Choose a reason for hiding this comment

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

Weird edge case: the user could set their defaultPatternType to structural. Then, hasValidPatternTypeFilter doesn't catch this. This probably never happens and not sure if it's worth fixing :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanhengl
Copy link
Member Author

Rebased due to merge conflict

@stefanhengl stefanhengl merged commit bc49218 into main Jun 26, 2024
15 checks passed
@stefanhengl stefanhengl deleted the sh/kw/cm branch June 26, 2024 09:59
peterguy pushed a commit that referenced this pull request Jun 26, 2024
This is part of the Keyword Search GA project (see background below). 

The core change is that we use the default pattern type consistently for
the query input field and preview. Before, we hardcoded `literal` as the
default and used `standard` for previews.

This is does not affect existing code monitors.

Other fixes:
- highlight keyword queries correctly

Background:
- "keyword" will soon be the new default pattern type
- the default pattern type can be overridden in the user/global settings
- query fields in all of our products should respect the default

Test Plan:
- The unit test is currently "skipped" with the following comment
```
// TODO: these tests trigger an error with CodeMirror, complaining about being
// loaded twice, see uiwjs/react-codemirror#506
```
- Manual testing:
- I created several code monitors with and without pattern type and
checked in the DB that the correct pattern type was appended.
- I configured a new default pattern type in my user settings and
verified that the setting changes the default pattern type for code
monitors.

Co-authored-by: Felix Kling <felix@felix-kling.de>
@sqs
Copy link
Member

sqs commented Jun 28, 2024

I believe I found an issue that is not caused by but is exposed by this PR: https://linear.app/sourcegraph/issue/SRCH-646/repo-and-other-filters-are-not-tokenized-in-query-input-for

@stefanhengl
Copy link
Member Author

I believe I found an issue that is not caused by but is exposed by this PR: https://linear.app/sourcegraph/issue/SRCH-646/repo-and-other-filters-are-not-tokenized-in-query-input-for

Thanks for reporting. Before this PR we didn't use the keyword chips for highlighting, so the issue was hidden. @fkling any idea what is happening?

@fkling
Copy link
Contributor

fkling commented Jun 28, 2024

I think we need to change the order in which these "chips" and the syntax highlighting is applied (i.e. change the order of the extensions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed team/product-platform team/search-platform Issues owned by the search platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants