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

Add support for local symf context #326

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Jan 22, 2024

Fixes #259

Test plan

Manual testing using full QA guide.

Other than that:

  1. Enable context

  2. Ask repo-specific question

  3. Check if response is based on the understanding of the local context

  4. Disable context

  5. Ask repo-specific question

  6. Check if response is lacking previous insight

  7. Click on refresh button and check if reindexing is in progress

  8. Click on help button and check if it opens proper documentation page

@pkukielka pkukielka changed the title [WIP] Add support for local symf context Add support for local symf context Jan 23, 2024
@pkukielka pkukielka force-pushed the pkukielka/add-support-for-symf branch from 19ab007 to 3ef379b Compare January 23, 2024 13:50
ApplicationManager.getApplication().executeOnPooledThread { refreshSubscriptionTab() }

ApplicationManager.getApplication().executeOnPooledThread {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to run it on a separate pooled thread than refreshSubscriptionTab() above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thank you! This is a merge error. It was supposed to be invokeLater!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I removed it altogether, it is not needed anymore after I discussed some design changes with Olaf.

when (val userObject = value.userObject) {
is Project -> {
textRenderer.appendHTML(
"<b>${userObject.name}</b> - <i>${userObject.basePath}</i>",
Copy link
Contributor

@mkondratek mkondratek Jan 23, 2024

Choose a reason for hiding this comment

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

  • we will need to verify it on the latest supported version of the IDE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks fine on 2023.1.1, 2023.2.2 and 2023.3.2.
2022.1 is fine as well.

tab.subscription.already-pro=(Already upgraded to Pro? Restart your IDE for changes to take effect)
context-panel.panel-name="Chat Context"
context-panel.reindex-button-name=""Trigger Reindexing""
context-panel.in-progress="Running Cody 'Keyword Search' indexer..."
Copy link
Contributor

Choose a reason for hiding this comment

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

  • pls double check the rendering of ' - sometimes it is needed to write '' to see a single '

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already fixed, thanks.

Copy link
Contributor

@exigow exigow left a comment

Choose a reason for hiding this comment

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

Can you please share some ideas for repo-specific questions? I'm trying to ask for smth like 'show me the class where service X is used' or 'list the classes that are written in Kotlin' or 'show me usages of X', but I'm missing something.

Also: I think that we should update testing guide with this feature ("Ask repo-specific question with local-only context").

Copy link
Contributor

@mkondratek mkondratek left a comment

Choose a reason for hiding this comment

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

lgtm

@pkukielka pkukielka force-pushed the pkukielka/add-support-for-symf branch from 7191a22 to 042f496 Compare January 23, 2024 16:47
Copy link
Contributor

@exigow exigow left a comment

Choose a reason for hiding this comment

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

From our private conversation, it emerged that to properly test, we must ask a precise question like "what is the first line of the CodyEditorFactoryListener class?". I was able to correctly test this feature with this tip.

However, I have two small topics:

  • We should bump the version to the Cody commit if this feature requires the latest version. I had to manually bump the version, and it was not obvious to me. It seems to me that developers should not be expected to work on CODY_DIR that is built on the latest main, especially since we often have to go back in history. This causes some conflicts on merge, but this can be simply fixed.
  • We should add to testing.md information that to objectively test this feature, we must specify the question in precise way, e.g., "what is the first line of the CodyEditorFactoryListener class?". This also was not obvious to me.

Looks good! 👍

@exigow
Copy link
Contributor

exigow commented Jan 23, 2024

I've spotted some bug: if you click refresh while a response is still being generated, an error java.util.concurrent.CancellationException occurs. After this error, there are different problems with the chat.

@pkukielka pkukielka merged commit e9f4386 into main Jan 23, 2024
2 checks passed
@pkukielka pkukielka deleted the pkukielka/add-support-for-symf branch January 23, 2024 18: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.

Implement UI for managing local symf context and triggering the refresh
3 participants