Skip to content

Fix chat headers and make them configurable#42

Merged
joaopluigi merged 8 commits intomainfrom
fix-chat-headers
Sep 8, 2025
Merged

Fix chat headers and make them configurable#42
joaopluigi merged 8 commits intomainfrom
fix-chat-headers

Conversation

@joaopluigi
Copy link
Copy Markdown
Contributor

Context

Header ## 👤 You was not being displayed for user messages:

Before

Screenshot 2025-09-07 at 19 21 41

After

Screenshot 2025-09-07 at 19 21 06

After with custom headers

This PR also add the option to configure custom headers such as:

require('eca').setup({
  chat = {
    headers = {
      user = "> ",
      assistant = "",
    },
  },
})
Screenshot 2025-09-07 at 19 24 21

Copilot AI review requested due to automatic review settings September 7, 2025 22:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes missing user message headers in the chat interface and adds configuration options for customizing chat headers. The PR resolves an issue where user messages weren't displaying the "## 👤 You" header and introduces the ability to customize both user and assistant headers through configuration.

  • Fixed user message header display by ensuring user messages are properly added with headers
  • Added configurable chat headers through the config system with sensible defaults
  • Updated message rendering logic to use the configurable headers consistently

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
lua/eca/sidebar.lua Updated message handling to display user headers and use configurable headers from config
lua/eca/config.lua Added chat configuration section with default header settings
docs/configuration.md Added documentation for the new chat header configuration options

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread lua/eca/sidebar.lua
-- Only check for empty text
if not text or text == "" then
Logger.trace("Ignoring empty text response")
Logger.debug("Ignoring empty text response")
Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

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

[nitpick] This change from Logger.trace to Logger.debug increases log verbosity for what appears to be a common occurrence (empty text). Consider keeping it as trace level to avoid cluttering debug logs.

Suggested change
Logger.debug("Ignoring empty text response")
Logger.trace("Ignoring empty text response")

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

.trace does not exists at Logger

Comment thread lua/eca/sidebar.lua
break
end
end

Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

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

String comparison using line:sub(1, #assistant_header) could fail if assistant_header is empty (length 0). This would cause line:sub(1, 0) to return an empty string, potentially matching any line incorrectly. Add a guard to check if assistant_header is not empty before the comparison.

Suggested change
if assistant_header == "" then
return 0
end

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@joaopluigi joaopluigi Sep 7, 2025

Choose a reason for hiding this comment

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

I believe it should be able to match an empty line 🤔, this is iterating backwards so it is important that we match the first empty line it finds from the end to the beginning

@ericdallo
Copy link
Copy Markdown
Member

Can we have the default as your print of the custom headers? it's what other editors do, and TBH I doubt someone will customize that

@joaopluigi
Copy link
Copy Markdown
Contributor Author

Can we have the default as your print of the custom headers? it's what other editors do, and TBH I doubt someone will customize that

I want to test this for some time first before making it the default. The code has some logic to get the last assistant message based on the header, and I want to stress this behavior more in the cases where the assistant header is empty. WDYT?

Copy link
Copy Markdown
Member

@ericdallo ericdallo 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!

@joaopluigi joaopluigi merged commit 8e315fd into main Sep 8, 2025
3 checks passed
@joaopluigi joaopluigi deleted the fix-chat-headers branch September 8, 2025 12:01
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.

3 participants