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

fix: Missing context files #535

Merged
merged 8 commits into from
Feb 12, 2024
Merged

Conversation

mkondratek
Copy link
Contributor

@mkondratek mkondratek commented Feb 7, 2024

Reattempt of #520.

Fixes https://github.com/sourcegraph/sourcegraph/issues/57378.
Fixes #328.

Test plan

  • test chat, test chat after restart, test commands

@mkondratek mkondratek self-assigned this Feb 7, 2024
@mkondratek mkondratek changed the title wip fix: Missing context files Feb 7, 2024
@mkondratek mkondratek force-pushed the mkondratek/fix/missing-context-files branch 4 times, most recently from 69839ec to 8f8e060 Compare February 8, 2024 21:41
if (extensionMessage.chatID != null) {
if (prevLastMessage != null) {
if (lastMessage?.contextFiles != messages.lastOrNull()?.contextFiles) {
val index = restoredMessagesCount + extensionMessage.messages.count() - 2
Copy link
Contributor

@pkukielka pkukielka Feb 9, 2024

Choose a reason for hiding this comment

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

I was always confused by that - 2 but I finally get it.
Won't it be the same (but IMHO more easier to understand) if instead you will use messages.count() - 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extensionMessage.messages always contains the full transcript of a given session.

Hence,
messages.count() - 1 is always the index of the last element
and
messages.count() - 2 is always the index of the element before the last element

That's all 😃

commandId.displayName,
),
chatSession.messages.count())
chatSession.addMessageAtIndex(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we force shouldAddBlinkingCursor = true there?
It will guarantee that cursor started blinking even if there is no response from the agent yet, it will be more responsive in some scenarios (e.g restart of the agent, slow LLM, bad connectivity)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but I tested it with no interent connection and it does not show the cursor (removes it immedietaly).

cancellationToken.onFinished {
      ApplicationManager.getApplication().invokeLater {
        removeBlinkingCursor()
        getLastMessage()?.onPartFinished()
      }
    }

cancellationToken removes it when there is not internet

@@ -28,6 +28,8 @@ class AccordionSection(title: String) : JPanel() {
contentPanel.isVisible = true
toggleButton.text = createToggleButtonHTML(sectionTitle, false)
}
revalidate()
Copy link
Contributor

Choose a reason for hiding this comment

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

What scenario was broken without that?

@mkondratek mkondratek force-pushed the mkondratek/fix/missing-context-files branch from 8f8e060 to 81333cd Compare February 9, 2024 12:22
@@ -28,6 +28,7 @@ private constructor(
private val project: Project,
newSessionId: CompletableFuture<SessionId>,
private val internalId: String = UUID.randomUUID().toString(),
private val restoredMessagesCount: Int = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be never updated, I guess it's not needed at all now?

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM except one comment I left.

@mkondratek mkondratek force-pushed the mkondratek/fix/missing-context-files branch from 81333cd to 41563ec Compare February 12, 2024 12:24
project,
sessionId,
state.internalId!!,
restoredMessagesCount = state.messages.count())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkukielka, pls take a look - the value is not updated, it's initialized here

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.

Looks good! 👍 I tested this on different repositories. It also works well with @.

@mkondratek mkondratek force-pushed the mkondratek/fix/missing-context-files branch from 19269dc to b15e831 Compare February 12, 2024 14:23
@mkondratek mkondratek force-pushed the mkondratek/fix/missing-context-files branch from 77af739 to 00da372 Compare February 12, 2024 15:43
Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@mkondratek mkondratek merged commit 47e8c12 into main Feb 12, 2024
2 checks passed
@mkondratek mkondratek deleted the mkondratek/fix/missing-context-files branch February 12, 2024 15:56
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.

Context files are not showing up in the chat JetBrains: context files in chat are not clickable
3 participants