Skip to content

Commit d1f1c74

Browse files
ethanyhouethanyhou
committed
Address comments.
1 parent a20bb35 commit d1f1c74

3 files changed

Lines changed: 101 additions & 8 deletions

File tree

‎com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingBlockParseTest.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT license.
3+
4+
package com.microsoft.copilot.eclipse.ui.chat;
5+
6+
import static org.junit.jupiter.api.Assertions.assertEquals;
7+
import static org.junit.jupiter.api.Assertions.assertNull;
8+
9+
import java.lang.reflect.Method;
10+
import java.util.List;
11+
12+
import org.junit.jupiter.api.Test;
13+
14+
/**
15+
* Unit tests for the private static parsing helpers in {@link ThinkingBlock}: {@code stripTrailingNewlines} and
16+
* {@code parseSections}. Exercises the helpers via reflection so the production visibility stays untouched.
17+
*
18+
* <p>Primary goal: guard CRLF handling so a {@code \r\n}-terminated body or title boundary does not leak a stray
19+
* {@code \r} into the rendered text.
20+
*/
21+
class ThinkingBlockParseTest {
22+
23+
@Test
24+
void stripTrailingNewlines_stripsLfCrAndCrlf() throws Exception {
25+
assertEquals("body", invokeStripTrailingNewlines("body\n"));
26+
assertEquals("body", invokeStripTrailingNewlines("body\r"));
27+
assertEquals("body", invokeStripTrailingNewlines("body\r\n"));
28+
assertEquals("body", invokeStripTrailingNewlines("body\r\n\r\n"));
29+
assertEquals("body", invokeStripTrailingNewlines("body\n\r\n"));
30+
assertEquals("body", invokeStripTrailingNewlines("body"));
31+
assertEquals("", invokeStripTrailingNewlines(""));
32+
assertEquals("", invokeStripTrailingNewlines("\r\n\r\n"));
33+
// Internal newlines and leading whitespace must survive untouched.
34+
assertEquals(" code\n more", invokeStripTrailingNewlines(" code\n more\r\n"));
35+
}
36+
37+
@Test
38+
void parseSections_handlesCrlfTitleBoundary() throws Exception {
39+
String raw = "intro body\r\n**Plan**\r\nplan body\r\n**Next**\r\nnext body\r\n";
40+
List<?> sections = invokeParseSections(raw);
41+
assertEquals(3, sections.size());
42+
43+
assertNull(title(sections.get(0)));
44+
assertEquals("intro body", body(sections.get(0)));
45+
46+
assertEquals("Plan", title(sections.get(1)));
47+
assertEquals("plan body", body(sections.get(1)));
48+
49+
assertEquals("Next", title(sections.get(2)));
50+
assertEquals("next body", body(sections.get(2)));
51+
}
52+
53+
@Test
54+
void parseSections_handlesLfOnlyTitleBoundary() throws Exception {
55+
// Existing LF behavior must remain unchanged.
56+
String raw = "intro\n**Plan**\nplan body";
57+
List<?> sections = invokeParseSections(raw);
58+
assertEquals(2, sections.size());
59+
assertNull(title(sections.get(0)));
60+
assertEquals("intro", body(sections.get(0)));
61+
assertEquals("Plan", title(sections.get(1)));
62+
assertEquals("plan body", body(sections.get(1)));
63+
}
64+
65+
private static String invokeStripTrailingNewlines(String input) throws Exception {
66+
Method m = ThinkingBlock.class.getDeclaredMethod("stripTrailingNewlines", String.class);
67+
m.setAccessible(true);
68+
return (String) m.invoke(null, input);
69+
}
70+
71+
private static List<?> invokeParseSections(String raw) throws Exception {
72+
Method m = ThinkingBlock.class.getDeclaredMethod("parseSections", String.class);
73+
m.setAccessible(true);
74+
return (List<?>) m.invoke(null, raw);
75+
}
76+
77+
private static String title(Object parsedSection) throws Exception {
78+
Method m = parsedSection.getClass().getDeclaredMethod("title");
79+
m.setAccessible(true);
80+
return (String) m.invoke(parsedSection);
81+
}
82+
83+
private static String body(Object parsedSection) throws Exception {
84+
Method m = parsedSection.getClass().getDeclaredMethod("body");
85+
m.setAccessible(true);
86+
return (String) m.invoke(parsedSection);
87+
}
88+
}

‎com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingBlock.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,9 @@
3232
* <p>Pure view: callers drive the visual state via {@link #showCompleted(String)} and {@link #showCancelled()}.
3333
* The owning turn widget is responsible for cancellation events and title fetching.
3434
*/
35-
class ThinkingBlock extends Composite {
35+
public class ThinkingBlock extends Composite {
3636
private static final String SECONDARY_TEXT_CSS_CLASS = "text-secondary";
37-
private static final Pattern TITLE_PATTERN = Pattern.compile("\\*\\*([^*\\n]+?)\\*\\*(?=\\n|$)");
37+
private static final Pattern TITLE_PATTERN = Pattern.compile("\\*\\*([^*\\r\\n]+?)\\*\\*(?=\\r?\\n|$)");
3838

3939
private Composite header;
4040
private GridLayout headerLayout;
@@ -122,7 +122,6 @@ public void showCancelled() {
122122
}
123123
if (!iconLabel.isDisposed()) {
124124
iconLabel.setImage(cancelledIcon);
125-
iconLabel.requestLayout();
126125
}
127126
setTitleText(Messages.thinking_cancelledTitle);
128127
setExpanded(false);
@@ -295,7 +294,7 @@ private static List<ParsedSection> parseSections(String raw) {
295294
currentTitle = matcher.group(1).trim();
296295
cursor = matcher.end();
297296
// Swallow the trailing newline(s) after the title so they don't show up at the top of the body.
298-
while (cursor < raw.length() && raw.charAt(cursor) == '\n') {
297+
while (cursor < raw.length() && (raw.charAt(cursor) == '\n' || raw.charAt(cursor) == '\r')) {
299298
cursor++;
300299
}
301300
}
@@ -309,7 +308,11 @@ private static List<ParsedSection> parseSections(String raw) {
309308

310309
private static String stripTrailingNewlines(String s) {
311310
int end = s.length();
312-
while (end > 0 && s.charAt(end - 1) == '\n') {
311+
while (end > 0) {
312+
char c = s.charAt(end - 1);
313+
if (c != '\n' && c != '\r') {
314+
break;
315+
}
313316
end--;
314317
}
315318
return s.substring(0, end);
@@ -397,7 +400,6 @@ private void updateChevron() {
397400
if (titleViewer != null && !titleViewer.getTextWidget().isDisposed()) {
398401
titleViewer.getTextWidget().setToolTipText(tooltip);
399402
}
400-
chevronLabel.requestLayout();
401403
}
402404

403405
private void refreshEnclosingScroller() {

‎com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingTurnWidget.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public void sealThinking() {
9494
hasTitles ? titles : null);
9595
ls.generateThinkingTitle(params)
9696
.thenAccept(resp -> SwtUtils.invokeOnDisplayThread(() -> {
97-
if (target.isDisposed() || target.isFinalized()) {
97+
if (isDisposed() || target.isDisposed() || target.isFinalized()) {
9898
return;
9999
}
100100
if (resp != null && StringUtils.isNotBlank(resp.title())) {
@@ -109,7 +109,10 @@ public void sealThinking() {
109109

110110
@Override
111111
protected void onChatMessageCancelled() {
112-
SwtUtils.invokeOnDisplayThread(() -> {
112+
SwtUtils.invokeOnDisplayThreadAsync(() -> {
113+
if (isDisposed()) {
114+
return;
115+
}
113116
if (currentBlock != null && !currentBlock.isDisposed() && !currentBlock.isFinalized()) {
114117
currentBlock.showCancelled();
115118
requestLayout();

0 commit comments

Comments
 (0)