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

openctx: set query label if provided #4604

Merged
merged 2 commits into from
Jun 19, 2024
Merged

openctx: set query label if provided #4604

merged 2 commits into from
Jun 19, 2024

Conversation

keegancsmith
Copy link
Member

@keegancsmith keegancsmith commented Jun 18, 2024

This sets the query label to the newly added "label" field in OpenCtx. This is a field which prompts the user what to type in an at mention.

Test Plan: Tested manually the following cases

  • non-openctx providers have same behaviour (no header)
  • openctx provider has loading display before query label (see devdocs below)
  • openctx query label goes away once query is being typed
  • openctx provider which doesn't set label shows "Search..." (see notion below)
  • web provider has updated label
2024-06-19-openctx-label.mp4

Parent #4621

Fixes https://linear.app/sourcegraph/issue/CODY-2275/add-all-the-right-labels-for-provider-mentions-query-stats

@keegancsmith keegancsmith changed the base branch from k/openctx-update to main June 19, 2024 10:08
@keegancsmith keegancsmith force-pushed the k/label branch 2 times, most recently from bd1dd47 to 92f7177 Compare June 19, 2024 11:22
@keegancsmith keegancsmith changed the base branch from main to k/required June 19, 2024 11:22
@keegancsmith keegancsmith marked this pull request as ready for review June 19, 2024 11:28
@keegancsmith keegancsmith requested review from a team June 19, 2024 11:28
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

This is great!

Seeing it IRL I think the URL ones might need to say “Type or paste a URL…” because otherwise it feels like you paste them into the popover or something.

I still think it’s a mistake we don’t have “Search files…” and “Search symbols…” for the built-in files and symbols. Would be great to re-add imo.

Base automatically changed from k/required to main June 19, 2024 13:20
@keegancsmith
Copy link
Member Author

Seeing it IRL I think the URL ones might need to say “Type or paste a URL…” because otherwise it feels like you paste them into the popover or something.

Done

I still think it’s a mistake we don’t have “Search files…” and “Search symbols…” for the built-in files and symbols. Would be great to re-add imo.

Will follow-up in another PR

@keegancsmith keegancsmith merged commit a5f917c into main Jun 19, 2024
19 checks passed
@keegancsmith keegancsmith deleted the k/label branch June 19, 2024 13:54
@keegancsmith
Copy link
Member Author

I still think it’s a mistake we don’t have “Search files…” and “Search symbols…” for the built-in files and symbols. Would be great to re-add imo.

So just to confirm, you want to make these slightly different to OpenCtx? For OpenCtx this is the behaviour:

  • Loading :: "meta.name" / Loading
  • No query and no items :: "meta.name" / "meta.mentions.label"
  • Query :: "meta.name" / items...

This is the current file behaviour:

  • Loading :: Loading
  • No query and no items :: Search for a file to include...
  • items :: items...

The current behaviour kinda makes sense. Because we kinda insta-load symbols and files I'm not sure we need a prompt, but it would make sense from a consistency point of view. To help make this decision I greatly improved the cases in our story book. Check this PR out. #4624

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