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

Enterprise repo picker follow up polish #1322

Closed
2 of 13 tasks
dominiccooney opened this issue Apr 16, 2024 · 15 comments · Fixed by #1669
Closed
2 of 13 tasks

Enterprise repo picker follow up polish #1322

dominiccooney opened this issue Apr 16, 2024 · 15 comments · Fixed by #1669

Comments

@dominiccooney
Copy link
Contributor

dominiccooney commented Apr 16, 2024

These are follow up issues from #982 and code review feedback on #1308

Tasks

  1. clients/jetbrains

Tooltip text:

About chat context
Select up to 10 repositories from [customer_instance_url] to use as reference context in this chat.
 
[Context Docs](https://sourcegraph.com/docs/cody/core-concepts/context)

Tree view language:

Chat Context X repos on [instance]
    Repositories
         example/example
         example/example2
@danielmarquespt
Copy link
Contributor

Added Make it accept full URLs and strip the https:// as a user just faced this problem and was unsure why the repo was not being accepted

@kalanchan
Copy link
Contributor

waiting on review

dominiccooney added a commit that referenced this issue May 16, 2024
Changes to the enterprise enhanced context panel:

- Pluralizes repo/repos in the repo tree view summary line
- Updates counts in the repo tree view summary line
- Mentions the number of ignored repos in the repo tree view summary
line (if any)
- Adds a help tooltip with a link to documentation
- Removes the help button from the right-hand side toolbar
- Clicking outside the repo picker popup accepts the edits instead of
cancelling them
- Alt-tabbing while the repo picker popup is open does not dismiss it

Part of #1322

## Test plan

Tested manually.

1. Run with `CODY_JETBRAINS_FEATURES=cody.feature.internals-menu=true`
2. Sign into an Enterprise account
3. Verify the repo picker is collapsed, one line and the edit pencil is
visible
4. Hover the repo picker and verify the tooltip, opening the link goes
to context documentation

![Screenshot 2024-05-10 at 20 23
52](https://github.com/sourcegraph/jetbrains/assets/55120/e3f30e14-a740-445d-bdeb-510cd98e98a8)

5. Add repos and verify the summary line language updates, pluralization
is correct, etc.
6. Use Internals, Testing: Cody Ignore and ignore some repositories,
check the "ignored" language updates, etc.

![Screenshot 2024-05-10 at 20 25
19](https://github.com/sourcegraph/jetbrains/assets/55120/023d38f7-6f41-4563-87ba-1d7093c796bd)

7. Verify the details of adding repositories with the pencil:
    - Change text, press ESC, repo list should *not* update
    - Change text, press cmd-enter, repo list should update
- Change text, click outside popup, repo list *should* update (new
behavior)

8. Open the pencil popup, verify that cmd-tab/alt-tab away from IntelliJ
and back does not close the popup
@toolmantim
Copy link
Contributor

I've jumped in here and done a review at the request of @dominiccooney so he can get started on the problems that were raised in the call last night, as he needed to get started on the eng side. Apologies for the dump here, just wanting to get them out quickly…


  1. The treeview items should match what’s added in the edit textarea 1:1 (aside from duplicates)
    • e.g. if you add “dev.azure.com/sourcegraph-mike/Test Project/repo1“ on s2, it doesn’t appear in the treeview at all with no feedback as to why
    • Instead, repos that aren’t valid are shown as disabled in the treeview, with a small message as to why they are ignored

Currently:

CleanShot 2024-05-17 at 12 45 42@2x

CleanShot 2024-05-17 at 12 45 55@2x

Instead here is an updated design that shows a few tweaks things labels, unnecessary nestings, unnecessary server name etc, as well as the invalid repo:

https://www.figma.com/design/m9Yvgv6W3aPRLfXnxgwnEz/JetBrains?node-id=1540-6065&t=tnA1l77M1NjRHLQh-4


  1. If the list is edited (e.g. the list is reordered, a new one is added), we retain the unchecked state of any repos in the treeview (I.e. it doesn’t just reset them all to be checked).
    • Removing a repo and re-adding it as two steps doesn’t need to remember the unchecked state, it should go back to being checked.

  1. Position the context tooltip above the treeview section, never overlapping it

  1. Nit 1: Clicking inside the input below the last line should put caret at the end of the last char on the last line
CleanShot.2024-05-17.at.12.34.44.mp4

  1. Nit 2: More top and bottom padding around the treeview so it's centred

CleanShot 2024-05-17 at 14 22 39@2x


Longer-term I think it's probably worth asking: is this the right type of UI here, or should we investigate a probably-more-easy-to-use dialog for doing this… something more like:

CleanShot 2024-05-17 at 14 06 35@2x

Be curious to see how people feel about the current UI, and @danielmarquespt we could work on a different version of it.

@danielmarquespt
Copy link
Contributor

thank you @toolmantim for jumping in on this.

I agree with the things you suggested, but I'll be conducting a review myself and share (other?) things that I might find, as well as the priority for them

@danielmarquespt
Copy link
Contributor

danielmarquespt commented May 17, 2024

List of improvements by priority UPDATED 21/05

(contains reviewed suggestions from @toolmantim and more)

  • Rearrange the tree view to this:
Screenshot 2024-05-21 at 15 04 51

Needed icons:
icons_reposelector.zip

1st line enables or disables all of the checkboxes below them
The inferred repo has the checkbox disabled
The counter of selected repos from [instance] moves from the first line to the node that represents external repos


  • Move the edit action from the sidebar to the last line of the tree

  • Add "Type to add repos from sourcegraph.sourcegraph.com" to the repo text area
Screenshot 2024-05-21 at 14 54 20
  • Add a import com.intellij.ui.TitledSeparator to the top of the repo selector
Screenshot 2024-05-21 at 14 52 11

string: Chat Context Settings


  • Remove tooltip from treeview and move it to the "Chat Context Settings" separator (on hover)

  • There is 1 to 1 parity between the entries on both

Removing an item from the text box, removes it from the tree. Used to be always true, but I'm seeing weird states there tree nodes still pressist after being removed from the text box


  • Warnings are applied to both the box and the list
Screenshot 2024-05-17 at 17 59 55

Invalid repos display the full string on the tree (since can be some gibberish or invalid urls, we can’t format it properly, so we can display that string with a ? icon)


  • Edit box needs to accepts full urls

like https://github.com/sourcegraph/jetbrains/ → adds to the tree as sourcegraph/jetbrains


  • Retain the unchecked state of any repos

If the list is edited (e.g. the list is reordered, a new one is added), we retain the unchecked state of any repos in the treeview (I.e. it doesn’t just reset them all to be checked).
Removing a repo and re-adding it as two steps doesn’t need to remember the unchecked state, it should go back to being checked.


  • Clicking inside the input below the last line should put caret at the end of the last char on the last line
CleanShot.2024-05-17.at.12.34.44.mp4

(UI details and fixes coming in a different issue. Functionality first)

@toolmantim
Copy link
Contributor

toolmantim commented May 20, 2024

@danielmarquespt "Local project" is a problematic label, because although the repo is detected from the local project, the code is all coming from the remote repo… no local git changes are included (which is a problem/surprise for a lot of users currently… this could make it worse). I think "Project repository" is probably a less problematic label.

Given the above… and that all the repos are from the instance… I think we can do without the Enterprise instance URL in the treeview. Maybe in the edit window is the right place to explain they can only add repos that exist on their instance?

Given all that… I think we can remove the extra level of tree nesting (Figma):

I'd suggest something like this for initial state: (last item opens edit view when clicked)

image

If > 1 extra repo: (last item opens edit view when clicked)

image

Collapsed view showing a count and some state, so when it's collapsed you can still tell what the scope of the chat is:

image

@danielmarquespt
Copy link
Contributor

Ok I just tested because I was though that the source of the local project was the local files. I tested with a local project that is not part of the sourcegraph instance and Cody kinda works, and is able to fecth files but its way more limited.

Given that I think it makes sense to put the local project under the instance node, and we can simplify the tree.

I'll update this


Regarding having the action at the bottom of the tree, I like it, but as far as I understand we cannot include actions as part of tree nodes. Maybe we can listen for a click there and trigger the edit box, but I'm not sure of much of an hack that would be

@toolmantim
Copy link
Contributor

If it's anything like VS Code it'll have a click handler you can trigger anything off.

@dominiccooney
Copy link
Contributor Author

dominiccooney commented May 21, 2024

There's multiple lists here with some conflicting details (Repos? Repositories? ... Say the remote name? Don't say the remote name?) and some of the list items are duplicates (for example #1354 tracks supporting https clone URLs.)

I am making a clean up pass in dpc/issue-1322 with these changes (so far):

  • Add the pencil/plus item, with different language for add vs edit, and have it trigger the popup.
  • Make the tree view single selection, which makes clicking on the add/edit more natural.
  • Fix the shade of icons; add dark variants.
  • Add the "Project repository" label on the automatically included repository.
  • Add the "Ignored by admin" label on the automatically included repository.
  • Simplify the counters to just "1 active repository", etc.
  • Position the popup above the repo list, instead of obscuring it (JetBrains: Edit remote repo popup is hidden behind the Windows taskbar. #1554)
  • Endpoint name in the repo editor popup

TODO:

  • Temporarily back to uncheck to remove, while I work on the model.

@toolmantim
Copy link
Contributor

@dominiccooney so @danielmarquespt and I just caught up, and Figma now has the latest changes with some simplifications.

@danielmarquespt
Copy link
Contributor

Updated the task list #1322 (comment)

@dominiccooney
Copy link
Contributor Author

@danielmarquespt , bit of feedback based on the design I had been implementing: the "active" language looks weird to me when enhanced context is unchecked. The repositories aren't "active" in any sense then:

Screenshot 2024-05-23 at 8 26 42

@toolmantim , when you say "the" Figma is that the link in this comment and this comment ... that still has some $placeholders.

@toolmantim
Copy link
Contributor

I did a search in the Figma for those strings, and found https://www.figma.com/design/m9Yvgv6W3aPRLfXnxgwnEz/JetBrains?node-id=1562-1092&m=dev which I think might be the current version of @danielmarquespt’s designs

@dominiccooney
Copy link
Contributor Author

Thanks @toolmantim , noted... next time I will search for the strings. Did not think of that!

@danielmarquespt
Copy link
Contributor

Thats a good point. I think we can remove the active and go with just "☑️ Enhanced Context 3 repos"

pkukielka pushed a commit that referenced this issue May 28, 2024
Fixes #1322, fixes #1425, fixes #1544, fixes #1532, fixes #1542

To summarize the changes:

- There are some repo resolution caches to make checking and unchecking
faster.
- The summary line has been simplified from counting total, ignored,
etc. repos to a simple count of repos which will be used (that is, the
enabled and not ignored repos, including the automatically included repo
if it is not ignored.)
- The automatically included repository is represented in the tree view
with a "Project repository" label.
- There's a separator between the tree view and the rest of the chat
panel. There's an expansive tooltip when you hover the separator, but
not the tree view so the tooltip does not impede expanding and
collapsing the tree view.
- The right hand side toolbar is gone, instead, you click on a tree view
item to bring up the repo list editor. You can also highlight it with
the keyboard and hit "enter".
- If you try to enable more than 10 repositories, you get feedback in
the form of an error notification.
- You can select and deselect repositories and they stay in the
repository tree view and are saved in chat state.
- The tree view reflects what you wrote in the popup. "Not found"
repositories are present with a label. You can delete a repository from
the text box to remove it from the tree view.
- The contrast and consistency of icons have been improved.
- The popup is positioned above the repository list, and is larger.
- The intermediate "Repositories" node of the tree view has been
removed.

Known bugs/caveats:

- When selecting/deselecting repositories in the tree view with the
keyboard, the item loses focus as the view is reconstructed.
- You can add an eleventh repository by specifying 10 repositories that
are not the automatically included repository. This one goes up to 11.
- Loading a chat with a de-selecting repository that has since been
filtered and checking it will result in the "ignored" state appearing.
This is because Cody Ignore is applied at late stage of remote repo
handling.
- Some of the new icons proposed in the design are not incorporated.
There are many overlapping versions of the design for this component...
I have to draw a line under this and handle any other feedback as
follow-ups.
- This does not address the https feedback in issue #1322; https URLs
are filed in #1354 and will be looked at separately.
- Repositories where the entered spec and the resolved name are
different may present as duplicates with one "not found."

## Test plan

Tested locally

![Screenshot 2024-05-28 at 19 19
09](https://github.com/sourcegraph/jetbrains/assets/55120/1640c6ca-5672-45dc-9f9e-617dfba1966d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants