Skip to content

Conversation

@brandonskiser
Copy link
Contributor

@brandonskiser brandonskiser commented Apr 10, 2025

Description of changes:

  • Addressing comments raised in feat: Introduce /compact command #1128
  • More accurately estimate the character count calculation for the conversation history
  • Update some UI points
  • Other misc code refactoring

Couple issues remain:

  1. There are two consecutive AssistantResponseMessages in the history after running /compact. This is because we push the assistant message on ResponseEvent::EndStream and again later in the same function. We don't get any validation errors for this, so this is apparently acceptable for now.
  2. The calculate_char_count function does not take context files into account. This is because we send the result of as_sendable_conversation_state to the model which includes the result of context_messages, but calculate_char_count does not use this output. Instead, as_sendable_conversation_state should be heavily refactored to not mutate ConversationState, so we can decouple our actual request data from the internal app representation (ie, ConversationState) more appropriately.

Since we only underestimate rather than overestimate token count, this should still be fine at the moment.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 79.45205% with 15 lines in your changes missing coverage. Please review.

Project coverage is 13.94%. Comparing base (d13b4c1) to head (6edcfd8).

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/mod.rs 29.41% 12 Missing ⚠️
crates/q_cli/src/cli/chat/conversation_state.rs 94.64% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
+ Coverage   13.92%   13.94%   +0.02%     
==========================================
  Files        2363     2363              
  Lines      204441   204483      +42     
  Branches   184805   184847      +42     
==========================================
+ Hits        28466    28523      +57     
+ Misses     174575   174560      -15     
  Partials     1400     1400              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brandonskiser brandonskiser force-pushed the bskiser/compact-refactor branch from 20e379e to 6edcfd8 Compare April 10, 2025 03:51
@brandonskiser brandonskiser marked this pull request as ready for review April 10, 2025 04:16
@brandonskiser brandonskiser requested a review from a team April 10, 2025 04:16
@brandonskiser brandonskiser force-pushed the bskiser/compact-refactor branch from 6edcfd8 to 6312f7b Compare April 10, 2025 04:18
@brandonskiser brandonskiser merged commit 5625410 into main Apr 10, 2025
19 of 20 checks passed
@brandonskiser brandonskiser deleted the bskiser/compact-refactor branch April 10, 2025 04:18
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