-
Notifications
You must be signed in to change notification settings - Fork 13.1k
fix(core): Improve compression message clarity for small history cases #4404
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
Changes from all commits
2fdad50
cecafe0
fcf9a74
7b28b4a
34f3409
b5be8ca
1e675d1
8d3b727
190a943
626a572
9eec471
8b0f3ac
e1ef6d5
1f9c6b0
718fc7e
65ca8c2
588dc56
80afef0
85e6947
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Google LLC | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { render } from 'ink-testing-library'; | ||
| import type { CompressionDisplayProps } from './CompressionMessage.js'; | ||
| import { CompressionMessage } from './CompressionMessage.js'; | ||
| import { CompressionStatus } from '@google/gemini-cli-core'; | ||
| import type { CompressionProps } from '../../types.js'; | ||
| import { describe, it, expect } from 'vitest'; | ||
|
|
||
| describe('<CompressionMessage />', () => { | ||
| const createCompressionProps = ( | ||
| overrides: Partial<CompressionProps> = {}, | ||
| ): CompressionDisplayProps => ({ | ||
| compression: { | ||
| isPending: false, | ||
| originalTokenCount: null, | ||
| newTokenCount: null, | ||
| compressionStatus: CompressionStatus.COMPRESSED, | ||
| ...overrides, | ||
| }, | ||
| }); | ||
|
|
||
| describe('pending state', () => { | ||
| it('renders pending message when compression is in progress', () => { | ||
| const props = createCompressionProps({ isPending: true }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain('Compressing chat history'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('normal compression (successful token reduction)', () => { | ||
| it('renders success message when tokens are reduced', () => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: 100, | ||
| newTokenCount: 50, | ||
| compressionStatus: CompressionStatus.COMPRESSED, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain('✦'); | ||
| expect(output).toContain( | ||
| 'Chat history compressed from 100 to 50 tokens.', | ||
| ); | ||
| }); | ||
|
|
||
| it('renders success message for large successful compressions', () => { | ||
| const testCases = [ | ||
| { original: 50000, new: 25000 }, // Large compression | ||
| { original: 700000, new: 350000 }, // Very large compression | ||
| ]; | ||
|
|
||
| testCases.forEach(({ original, new: newTokens }) => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: original, | ||
| newTokenCount: newTokens, | ||
| compressionStatus: CompressionStatus.COMPRESSED, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain('✦'); | ||
| expect(output).toContain( | ||
| `compressed from ${original} to ${newTokens} tokens`, | ||
| ); | ||
| expect(output).not.toContain('Skipping compression'); | ||
| expect(output).not.toContain('did not reduce size'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('skipped compression (tokens increased or same)', () => { | ||
| it('renders skip message when compression would increase token count', () => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: 50, | ||
| newTokenCount: 75, | ||
| compressionStatus: | ||
| CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain('✦'); | ||
| expect(output).toContain( | ||
| 'Compression was not beneficial for this history size.', | ||
| ); | ||
| }); | ||
|
|
||
| it('renders skip message when token counts are equal', () => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: 50, | ||
| newTokenCount: 50, | ||
| compressionStatus: | ||
| CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain( | ||
| 'Compression was not beneficial for this history size.', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('message content validation', () => { | ||
| it('displays correct compression statistics', () => { | ||
| const testCases = [ | ||
| { | ||
| original: 200, | ||
| new: 80, | ||
| expected: 'compressed from 200 to 80 tokens', | ||
| }, | ||
| { | ||
| original: 500, | ||
| new: 150, | ||
| expected: 'compressed from 500 to 150 tokens', | ||
| }, | ||
| { | ||
| original: 1500, | ||
| new: 400, | ||
| expected: 'compressed from 1500 to 400 tokens', | ||
| }, | ||
| ]; | ||
|
|
||
| testCases.forEach(({ original, new: newTokens, expected }) => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: original, | ||
| newTokenCount: newTokens, | ||
| compressionStatus: CompressionStatus.COMPRESSED, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain(expected); | ||
| }); | ||
| }); | ||
|
|
||
| it('shows skip message for small histories when new tokens >= original tokens', () => { | ||
| const testCases = [ | ||
| { original: 50, new: 60 }, // Increased | ||
| { original: 100, new: 100 }, // Same | ||
| { original: 49999, new: 50000 }, // Just under 50k threshold | ||
| ]; | ||
|
|
||
| testCases.forEach(({ original, new: newTokens }) => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: original, | ||
| newTokenCount: newTokens, | ||
| compressionStatus: | ||
| CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain( | ||
| 'Compression was not beneficial for this history size.', | ||
| ); | ||
| expect(output).not.toContain('compressed from'); | ||
| }); | ||
| }); | ||
|
|
||
| it('shows compression failure message for large histories when new tokens >= original tokens', () => { | ||
| const testCases = [ | ||
| { original: 50000, new: 50100 }, // At 50k threshold | ||
| { original: 700000, new: 710000 }, // Large history case | ||
| { original: 100000, new: 100000 }, // Large history, same count | ||
| ]; | ||
|
|
||
| testCases.forEach(({ original, new: newTokens }) => { | ||
| const props = createCompressionProps({ | ||
| isPending: false, | ||
| originalTokenCount: original, | ||
| newTokenCount: newTokens, | ||
| compressionStatus: | ||
| CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT, | ||
| }); | ||
| const { lastFrame } = render(<CompressionMessage {...props} />); | ||
| const output = lastFrame(); | ||
|
|
||
| expect(output).toContain('compression did not reduce size'); | ||
| expect(output).not.toContain('compressed from'); | ||
| expect(output).not.toContain('Compression was not beneficial'); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4,12 +4,12 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | ||||
| */ | ||||
|
|
||||
| import type React from 'react'; | ||||
| import { Box, Text } from 'ink'; | ||||
| import type { CompressionProps } from '../../types.js'; | ||||
| import Spinner from 'ink-spinner'; | ||||
| import { theme } from '../../semantic-colors.js'; | ||||
| import { SCREEN_READER_MODEL_PREFIX } from '../../textConstants.js'; | ||||
| import { CompressionStatus } from '@google/gemini-cli-core'; | ||||
|
|
||||
| export interface CompressionDisplayProps { | ||||
| compression: CompressionProps; | ||||
|
|
@@ -19,18 +19,46 @@ export interface CompressionDisplayProps { | |||
| * Compression messages appear when the /compress command is run, and show a loading spinner | ||||
| * while compression is in progress, followed up by some compression stats. | ||||
| */ | ||||
| export const CompressionMessage: React.FC<CompressionDisplayProps> = ({ | ||||
| export function CompressionMessage({ | ||||
| compression, | ||||
| }) => { | ||||
| const text = compression.isPending | ||||
| ? 'Compressing chat history' | ||||
| : `Chat history compressed from ${compression.originalTokenCount ?? 'unknown'}` + | ||||
| ` to ${compression.newTokenCount ?? 'unknown'} tokens.`; | ||||
| }: CompressionDisplayProps): React.JSX.Element { | ||||
| const { isPending, originalTokenCount, newTokenCount, compressionStatus } = | ||||
| compression; | ||||
|
|
||||
| const originalTokens = originalTokenCount ?? 0; | ||||
| const newTokens = newTokenCount ?? 0; | ||||
|
|
||||
| const getCompressionText = () => { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: (all messages) Let's prefer "Conversation" over "Chat" |
||||
| if (isPending) { | ||||
| return 'Compressing chat history'; | ||||
| } | ||||
|
|
||||
| switch (compressionStatus) { | ||||
| case CompressionStatus.COMPRESSED: | ||||
| return `Chat history compressed from ${originalTokens} to ${newTokens} tokens.`; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "Your conversation history was compressed from X to Y tokens" (drop the period, add "Your") |
||||
| case CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT: | ||||
| // For smaller histories (< 50k tokens), compression overhead likely exceeds benefits | ||||
| if (originalTokens < 50000) { | ||||
| return 'Compression was not beneficial for this history size.'; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
| } | ||||
| // For larger histories where compression should work but didn't, | ||||
| // this suggests an issue with the compression process itself | ||||
| return 'Chat history compression did not reduce size. This may indicate issues with the compression prompt.'; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anj-s this is probably the most controversial part of this change. Would be curious to hear your thoughts about whether it is useful for users to understand just how much compression would have increased size or whether this is sufficient.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When does this happen? I am not sure I understand this error case. Either way, I agree that this does not help the user. I would make sure to log this error on our end. What are some workarounds we can suggest to the user?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /me puts on UX hat. My suggestion here, citing this "writing styleguide" [1] (which I personally find to be the best/most-complete document that details how Google should provide error messaging in software)...
Reasons why:
[1] https://m2.material.io/design/communication/writing.html |
||||
| case CompressionStatus.COMPRESSION_FAILED_TOKEN_COUNT_ERROR: | ||||
| return 'Could not compress chat history due to a token counting error.'; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion:
My feelings:
|
||||
| case CompressionStatus.NOOP: | ||||
| return 'Chat history is already compressed.'; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip unnecessary punctuation, add "Your conversation history"
ref: https://m2.material.io/design/communication/writing.html#principles |
||||
| default: | ||||
| return ''; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use checkExhaustive here ref:
|
||||
| } | ||||
| }; | ||||
|
|
||||
| const text = getCompressionText(); | ||||
ShammiAnand marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
|
||||
| return ( | ||||
| <Box flexDirection="row"> | ||||
| <Box marginRight={1}> | ||||
| {compression.isPending ? ( | ||||
| {isPending ? ( | ||||
| <Spinner type="dots" /> | ||||
| ) : ( | ||||
| <Text color={theme.text.accent}>✦</Text> | ||||
|
|
@@ -48,4 +76,4 @@ export const CompressionMessage: React.FC<CompressionDisplayProps> = ({ | |||
| </Box> | ||||
| </Box> | ||||
| ); | ||||
| }; | ||||
| } | ||||
Uh oh!
There was an error while loading. Please reload this page.