-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(ui): add ui.history.collapsePreviewCount to show last N turns when resuming collapsed sessions #5848
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
feat(ui): add ui.history.collapsePreviewCount to show last N turns when resuming collapsed sessions #5848
Changes from all commits
7f36e32
0733644
0c7fb83
f921fee
c19c3c0
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 |
|---|---|---|
|
|
@@ -6,11 +6,12 @@ | |
|
|
||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { | ||
| applyCollapsePolicyAndSummary, | ||
| buildResumedHistoryItems, | ||
| stripSuppressOnRestore, | ||
| expandCollapsedHistory, | ||
| } from './resumeHistoryUtils.js'; | ||
| import { ToolCallStatus } from '../types.js'; | ||
| import { MessageType, ToolCallStatus } from '../types.js'; | ||
| import type { | ||
| AnyDeclarativeTool, | ||
| Config, | ||
|
|
@@ -496,6 +497,88 @@ describe('resumeHistoryUtils', () => { | |
| }); | ||
| }); | ||
|
|
||
|
Collaborator
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] Good unit test coverage for Consider adding at least one integration test (e.g., in — qwen3.7-max via Qwen Code /review |
||
| describe('applyCollapsePolicyAndSummary', () => { | ||
| const makeItems = (): HistoryItem[] => | ||
| [ | ||
| { id: 1, type: MessageType.USER, text: 'first' }, | ||
| { id: 2, type: MessageType.GEMINI, text: 'first response' }, | ||
| { id: 3, type: MessageType.USER, text: 'second' }, | ||
| { id: 4, type: MessageType.GEMINI, text: 'second response' }, | ||
| { id: 5, type: MessageType.USER, text: 'third' }, | ||
| { id: 6, type: MessageType.GEMINI, text: 'third response' }, | ||
| ] as HistoryItem[]; | ||
|
|
||
| const expectSuppressed = (item: HistoryItem) => { | ||
| expect(item.display).toEqual( | ||
| expect.objectContaining({ suppressOnRestore: true }), | ||
| ); | ||
| }; | ||
|
|
||
| const expectVisible = (item: HistoryItem) => { | ||
| expect(item.display?.suppressOnRestore).toBeUndefined(); | ||
| }; | ||
|
|
||
| it('suppresses all items and shows the full summary count by default', () => { | ||
| const result = applyCollapsePolicyAndSummary(makeItems(), true); | ||
|
|
||
| expect(result).toHaveLength(7); | ||
| result.slice(0, 6).forEach(expectSuppressed); | ||
| expect(result[6]).toEqual( | ||
| expect.objectContaining({ | ||
| id: 7, | ||
| type: MessageType.INFO, | ||
| text: expect.stringContaining('6 messages hidden'), | ||
| display: { kind: 'collapse-summary' }, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
|
Collaborator
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] The A test with it('shows all items without a summary when preview count exceeds user turns', () => {
const rawItems = makeItems(); // 3 user turns
const result = applyCollapsePolicyAndSummary(rawItems, true, 5);
expect(result).toEqual(rawItems);
result.forEach(expectVisible);
});— qwen3.7-max via Qwen Code /review |
||
| it('keeps the most recent N user turns visible and summarizes only hidden items', () => { | ||
|
Collaborator
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] No test covers it('keeps only the last user turn visible when previewCount is 1', () => {
const result = applyCollapsePolicyAndSummary(makeItems(), true, 1);
expect(result).toHaveLength(7);
result.slice(0, 4).forEach(expectSuppressed);
result.slice(4, 6).forEach(expectVisible);
expect(result[6]).toEqual(
expect.objectContaining({
text: expect.stringContaining('4 messages hidden'),
display: { kind: 'collapse-summary' },
}),
);
});— qwen3.7-max via Qwen Code /review |
||
| const result = applyCollapsePolicyAndSummary(makeItems(), true, 2); | ||
|
|
||
| expect(result).toHaveLength(7); | ||
| result.slice(0, 2).forEach(expectSuppressed); | ||
| result.slice(2, 6).forEach(expectVisible); | ||
| expect(result[6]).toEqual( | ||
| expect.objectContaining({ | ||
| id: 7, | ||
| type: MessageType.INFO, | ||
| text: expect.stringContaining('2 messages hidden'), | ||
| display: { kind: 'collapse-summary' }, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('shows all items without a summary when preview count covers all user turns', () => { | ||
| const rawItems = makeItems(); | ||
| const result = applyCollapsePolicyAndSummary(rawItems, true, 3); | ||
|
|
||
| expect(result).toEqual(rawItems); | ||
| expect( | ||
| result.some((item) => item.display?.kind === 'collapse-summary'), | ||
| ).toBe(false); | ||
| result.forEach(expectVisible); | ||
| }); | ||
|
|
||
| it('shows all items without a summary when preview count is -1', () => { | ||
| const rawItems = makeItems(); | ||
|
Collaborator
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] Test doesn't exercise leading non-USER items (common in real sessions) The Consider adding: it('still hides leading non-user items when preview count covers all user turns', () => {
const items = [
{ id: 0, type: MessageType.INFO, text: 'system context' },
...makeItems(),
] as HistoryItem[];
const result = applyCollapsePolicyAndSummary(items, true, 3);
expect(result[0].display).toEqual(
expect.objectContaining({ suppressOnRestore: true }),
);
expect(result).toHaveLength(8);
expect(result[7].text).toContain('1 messages hidden');
});— bailian/glm-5.2 via Qwen Code /review |
||
| const result = applyCollapsePolicyAndSummary(rawItems, true, -1); | ||
|
|
||
| expect(result).toBe(rawItems); | ||
| }); | ||
|
|
||
| it('returns raw items unchanged when collapseOnResume is false', () => { | ||
| const rawItems = makeItems(); | ||
| const result = applyCollapsePolicyAndSummary(rawItems, false, 1); | ||
|
|
||
| expect(result).toBe(rawItems); | ||
| }); | ||
|
|
||
| it('returns empty history without a summary', () => { | ||
| expect(applyCollapsePolicyAndSummary([], true)).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('stripSuppressOnRestore', () => { | ||
| it('returns item unchanged when display is undefined', () => { | ||
|
Collaborator
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. [Nice to have] No test exercises Consider adding a test case that constructs a partially-collapsed history (e.g., 2 suppressed + 4 visible + 1 collapse-summary) and asserts that — qwen3.7-max via Qwen Code /review |
||
| const item = { id: 1, type: 'user', text: 'hello' } as HistoryItem; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -568,16 +568,37 @@ export function expandCollapsedHistory(items: HistoryItem[]): HistoryItem[] { | |||||
| export function applyCollapsePolicyAndSummary( | ||||||
| rawItems: HistoryItem[], | ||||||
| collapseOnResume: boolean, | ||||||
| collapsePreviewCount: number = 0, | ||||||
| ): HistoryItem[] { | ||||||
| if (!collapseOnResume) return rawItems; | ||||||
| if (collapsePreviewCount === -1) return rawItems; | ||||||
|
|
||||||
| let boundary = rawItems.length; | ||||||
|
Collaborator
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] Negative values other than The
Suggested change
— bailian/glm-5.2 via Qwen Code /review |
||||||
| if (collapsePreviewCount > 0) { | ||||||
| let userTurnCount = 0; | ||||||
| for (let i = rawItems.length - 1; i >= 0; i--) { | ||||||
| if (rawItems[i].type === MessageType.USER) { | ||||||
| userTurnCount++; | ||||||
| if (userTurnCount === collapsePreviewCount) { | ||||||
| boundary = i; | ||||||
| break; | ||||||
| } | ||||||
|
Collaborator
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]
Suggested change
Using — qwen3.7-max via Qwen Code /review |
||||||
| } | ||||||
| } | ||||||
| if (userTurnCount < collapsePreviewCount) { | ||||||
| boundary = 0; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const uiHistoryItems = applyResumeDisplayPolicy(rawItems); | ||||||
| const hiddenItems = applyResumeDisplayPolicy(rawItems.slice(0, boundary)); | ||||||
| const visibleItems = rawItems.slice(boundary); | ||||||
| const uiHistoryItems = [...hiddenItems, ...visibleItems]; | ||||||
|
|
||||||
| if (rawItems.length > 0) { | ||||||
| if (boundary > 0) { | ||||||
| const nextId = rawItems[rawItems.length - 1].id + 1; | ||||||
| return [ | ||||||
| ...uiHistoryItems, | ||||||
| { id: nextId, ...createHistoryCollapseSummaryItem(rawItems.length) }, | ||||||
| { id: nextId, ...createHistoryCollapseSummaryItem(boundary) }, | ||||||
| ]; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Schema declares
type: 'number'but the boundary algorithm inapplyCollapsePolicyAndSummaryrequires an integer — it uses strict equality (userTurnCount === collapsePreviewCount) against an integer counter. A float like2.5silently produces incorrect behavior (the loop never matches, falls through to unexpected collapse).Other integer-valued settings in this same file use
jsonSchemaOverrideto enforce integer constraints (e.g.,quorumSizeat line 2288,stopHookBlockCapat line 2629,fileHistoryRetentionDaysat line 1568).— qwen3.7-max via Qwen Code /review