Skip to content

Commit 3452bac

Browse files
committed
fix(chat): supersede the row directly above, and never a user row
Three things the review caught, all real. A `running` row was left standing under the row that said how the compaction ended, because only a summary-only row was being replaced. Both were then visible, live. A terminal row now replaces a running one — and carries over the summary if one had already folded into it, which is the order running → summary → boundary that live delivery produces and that would otherwise have dropped the summary on the floor. The stray-copy scan could take a *user* row that happened to repeat the summary text. Assistant rows only. The orphan-summary scan reached backwards past intervening rows, so a compaction that arrived later could adopt a summary belonging to an earlier one. Only the row directly above is superseded now; a compaction further back belongs to itself. Four cases added: running → done, running → failed, running → summary → boundary keeping the summary, an orphan left alone behind an ordinary row, and a user echo of the summary text surviving. Two of them fail without the first fix. Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
1 parent 89920a9 commit 3452bac

2 files changed

Lines changed: 66 additions & 11 deletions

File tree

src/modules/chat/hooks/useChatMessages.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,8 @@ function appendCompactionRow(
124124
// directly above, so it is searched for — newest first, ordinary rows only.
125125
for (let index = converted.length - 1; index >= 0; index -= 1) {
126126
const row = converted[index];
127-
if (!row.compact && (row.content || '').trim() === text) {
127+
// Assistant rows only: a user who pastes the same text is not a duplicate.
128+
if (row.type === 'assistant' && !row.compact && (row.content || '').trim() === text) {
128129
converted.splice(index, 1);
129130
break;
130131
}
@@ -156,16 +157,19 @@ function appendCompactionRow(
156157
return false;
157158
}
158159

159-
// Live, the summary arrived before this row and made one of its own. Take it
160-
// back: this row says what that one could not.
161-
let summary: string | undefined;
162-
for (let index = converted.length - 1; index >= 0; index -= 1) {
163-
const row = converted[index];
164-
if (row.compact && row.compactSummary && !row.content) {
165-
summary = row.compactSummary;
166-
converted.splice(index, 1);
167-
break;
168-
}
160+
// This row supersedes the one directly above it in two cases, and only when it
161+
// is directly above — a compaction further back belongs to itself:
162+
//
163+
// - a `running` row, now that the compaction has finished or failed;
164+
// - the summary-only row a summary makes when it arrives before its
165+
// boundary, which is the live order. Its summary comes along, since this
166+
// row says what that one could not.
167+
const previous = converted[converted.length - 1];
168+
const supersedes = Boolean(previous?.compact)
169+
&& (previous.compact?.phase === 'running' || !previous.content);
170+
const summary = supersedes ? previous.compactSummary : undefined;
171+
if (supersedes) {
172+
converted.pop();
169173
}
170174

171175
converted.push({

src/modules/chat/tests/compactionRows.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,57 @@ describe('compaction rows', () => {
7171
]);
7272
});
7373

74+
it('replaces the running row with the row that says how it ended', () => {
75+
const running = () => row({ content: 'Compacting conversation…', compact: { phase: 'running' } });
76+
77+
expect(normalizedToChatMessages([running(), boundary()]))
78+
.toMatchObject([{ compact: { phase: 'done' } }]);
79+
80+
const failed = row({
81+
content: 'Compaction failed: context still too large',
82+
compact: { phase: 'failed', error: 'context still too large' },
83+
});
84+
expect(normalizedToChatMessages([running(), failed]))
85+
.toMatchObject([{ compact: { phase: 'failed' } }]);
86+
});
87+
88+
it('keeps the summary when it folded into a running row the boundary then replaced', () => {
89+
const converted = normalizedToChatMessages([
90+
row({ content: 'Compacting conversation…', compact: { phase: 'running' } }),
91+
summary(),
92+
boundary(),
93+
]);
94+
95+
expect(converted).toHaveLength(1);
96+
expect(converted[0].compact?.phase).toBe('done');
97+
expect(converted[0].compactSummary).toBe('The summary body');
98+
});
99+
100+
it('leaves an earlier compaction alone: only the row directly above is superseded', () => {
101+
const converted = normalizedToChatMessages([
102+
summary(),
103+
row({ content: 'on with the work' }),
104+
boundary(),
105+
]);
106+
107+
expect(converted).toHaveLength(3);
108+
expect(converted[0].compactSummary).toBe('The summary body');
109+
expect(converted[1].content).toBe('on with the work');
110+
expect(converted[2].compactSummary).toBeUndefined();
111+
});
112+
113+
it('never takes a user row for a stray copy of the summary', () => {
114+
const userEcho: NormalizedMessage = {
115+
...row({ content: 'The summary body' }),
116+
role: 'user',
117+
} as NormalizedMessage;
118+
119+
const converted = normalizedToChatMessages([userEcho, boundary(), summary()]);
120+
121+
expect(converted.map((message) => message.type)).toEqual(['user', 'assistant']);
122+
expect(converted[1].compactSummary).toBe('The summary body');
123+
});
124+
74125
it('keeps a running compaction, and its phase, so the row can say so', () => {
75126
const converted = normalizedToChatMessages([
76127
row({ content: 'Compacting conversation…', compact: { phase: 'running' } }),

0 commit comments

Comments
 (0)