Skip to content

Commit a20bb35

Browse files
committed
Address comments.
1 parent bca812e commit a20bb35

2 files changed

Lines changed: 68 additions & 19 deletions

File tree

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

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import com.microsoft.copilot.eclipse.ui.utils.UiUtils;
2828

2929
/**
30-
* Collapsible "Thinking" banner shown above an assistant turn while the model emits thinking deltas.
30+
* Collapsible "Thinking" banner shown above an assistant turn while the model emits thinking stream.
3131
*
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.
@@ -48,6 +48,14 @@ class ThinkingBlock extends Composite {
4848
private final StringBuilder textBuffer = new StringBuilder();
4949
private boolean expanded = true;
5050

51+
/**
52+
* Lifecycle of the block. Transitions are always forward: STREAMING → (SEALED →)? → COMPLETED|CANCELLED.
53+
* SEALED means {@code sealThinking()} has fired and a title fetch is in flight; new thinking stream fragments must
54+
* start a new block.
55+
*/
56+
private enum State { STREAMING, SEALED, COMPLETED, CANCELLED }
57+
58+
private State state = State.STREAMING;
5159
private SpinnerAnimator spinner;
5260
private Image cancelledIcon;
5361
private Image downArrowImage;
@@ -76,7 +84,7 @@ public ThinkingBlock(Composite parent, int style) {
7684
updateChevron();
7785
}
7886

79-
/** Append a thinking delta. Null/empty fragments are ignored. */
87+
/** Append a thinking stream fragment. Null/empty fragments are ignored. */
8088
public void appendText(String fragment) {
8189
if (fragment == null || fragment.isEmpty()) {
8290
return;
@@ -100,6 +108,7 @@ public void showCompleted(String title) {
100108
}
101109
setTitleText(title);
102110
setExpanded(false);
111+
state = State.COMPLETED;
103112
}
104113

105114
/** Swap to the cancel icon, set the cancelled title, and collapse. No-op if already finalized. */
@@ -117,11 +126,27 @@ public void showCancelled() {
117126
}
118127
setTitleText(Messages.thinking_cancelledTitle);
119128
setExpanded(false);
129+
state = State.CANCELLED;
120130
}
121131

122-
/** True once the spinner has stopped (block has been completed or cancelled). */
132+
/**
133+
* Mark the block as sealed: the owning widget has requested a title and any further thinking stream fragments must
134+
* land in a new block. No-op once the block has been finalized or already sealed.
135+
*/
136+
public void markSealed() {
137+
if (state == State.STREAMING) {
138+
state = State.SEALED;
139+
}
140+
}
141+
142+
/** True only while new thinking stream fragments should still be appended to this block. */
143+
public boolean isAcceptingThinkStream() {
144+
return state == State.STREAMING;
145+
}
146+
147+
/** True once the block has been completed or cancelled (spinner stopped, final title shown). */
123148
public boolean isFinalized() {
124-
return spinner == null;
149+
return state == State.COMPLETED || state == State.CANCELLED;
125150
}
126151

127152
/** The full accumulated thinking text streamed so far. */
@@ -191,8 +216,13 @@ public void mouseUp(MouseEvent e) {
191216
toggleExpanded();
192217
}
193218
};
219+
// Attach to every header child (and the header itself) so the entire area that shows the hand
220+
// cursor is actually clickable. iconLabel is intentionally excluded: it hosts the live spinner
221+
// animation (and the cancel icon afterwards), and a clickable spinner is an odd affordance.
222+
header.addMouseListener(toggleListener);
194223
titleText.addMouseListener(toggleListener);
195224
chevronLabel.addMouseListener(toggleListener);
225+
filler.addMouseListener(toggleListener);
196226
}
197227

198228
private void createBody() {
@@ -256,8 +286,10 @@ private static List<ParsedSection> parseSections(String raw) {
256286
String currentTitle = null;
257287
int cursor = 0;
258288
while (matcher.find()) {
259-
String body = raw.substring(cursor, matcher.start()).strip();
260-
if (currentTitle != null || !body.isEmpty()) {
289+
// Preserve the body's original whitespace (e.g. leading indentation for code blocks); only strip the
290+
// trailing newline(s) that visually separate the body from the upcoming title delimiter.
291+
String body = stripTrailingNewlines(raw.substring(cursor, matcher.start()));
292+
if (currentTitle != null || !body.isBlank()) {
261293
result.add(new ParsedSection(currentTitle, body));
262294
}
263295
currentTitle = matcher.group(1).trim();
@@ -267,13 +299,22 @@ private static List<ParsedSection> parseSections(String raw) {
267299
cursor++;
268300
}
269301
}
270-
String tail = raw.substring(cursor).strip();
271-
if (currentTitle != null || !tail.isEmpty()) {
302+
// Tail has no following title delimiter; only trim trailing newlines so leading indentation survives.
303+
String tail = stripTrailingNewlines(raw.substring(cursor));
304+
if (currentTitle != null || !tail.isBlank()) {
272305
result.add(new ParsedSection(currentTitle, tail));
273306
}
274307
return result;
275308
}
276309

310+
private static String stripTrailingNewlines(String s) {
311+
int end = s.length();
312+
while (end > 0 && s.charAt(end - 1) == '\n') {
313+
end--;
314+
}
315+
return s.substring(0, end);
316+
}
317+
277318
private void setTitleText(String text) {
278319
if (titleViewer == null || titleViewer.getTextWidget().isDisposed()) {
279320
return;

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

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import org.eclipse.swt.widgets.Composite;
99

1010
import com.microsoft.copilot.eclipse.core.CopilotCore;
11+
import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection;
1112
import com.microsoft.copilot.eclipse.core.lsp.protocol.GenerateThinkingTitleParams;
1213
import com.microsoft.copilot.eclipse.core.lsp.protocol.Thinking;
1314
import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager;
@@ -19,8 +20,6 @@
1920
public abstract class ThinkingTurnWidget extends BaseTurnWidget {
2021

2122
private ThinkingBlock currentBlock;
22-
/** True once {@link #sealThinking()} has fired for {@link #currentBlock}; the next delta will start a new block. */
23-
private boolean sealed;
2423

2524
/** Construct a turn widget that supports streaming thinking blocks. */
2625
protected ThinkingTurnWidget(Composite parent, int style, ChatServiceManager serviceManager, String turnId,
@@ -34,7 +33,7 @@ public ThinkingTurnWidget getActiveTurnWidget() {
3433
}
3534

3635
/**
37-
* Append a thinking delta from the language server, routing to the active turn (parent or subagent).
36+
* Append a thinking stream fragment from the language server, routing to the active turn (parent or subagent).
3837
* Must be called on the UI thread.
3938
*/
4039
public void appendThinking(Thinking thinking) {
@@ -48,9 +47,11 @@ public void appendThinking(Thinking thinking) {
4847
if (active == null || active.isDisposed()) {
4948
return;
5049
}
51-
if (active.currentBlock == null || active.currentBlock.isDisposed() || active.sealed) {
50+
// Single source of truth: ThinkingBlock decides whether it can accept more thinking stream fragments (it can't
51+
// once sealed, completed, or cancelled). Any of those transitions must start a fresh block.
52+
if (active.currentBlock == null || active.currentBlock.isDisposed()
53+
|| !active.currentBlock.isAcceptingThinkStream()) {
5254
active.currentBlock = new ThinkingBlock(active, SWT.NONE);
53-
active.sealed = false;
5455
}
5556
active.currentBlock.appendText(thinking.text());
5657
}
@@ -68,23 +69,30 @@ public void sealThinking() {
6869
return;
6970
}
7071
ThinkingBlock target = active.currentBlock;
71-
// Skip when already sealed, or when a prior cancel has already finalized the block (so we don't fire a stale
72-
// generateTitle request whose response would be discarded).
73-
if (target == null || target.isDisposed() || active.sealed || target.isFinalized()) {
72+
// Skip when the block is no longer accepting thinking stream fragments (already sealed, completed, or cancelled)
73+
// so we don't fire a stale generateTitle request whose response would be discarded.
74+
if (target == null || target.isDisposed() || !target.isAcceptingThinkStream()) {
7475
return;
7576
}
7677
String content = target.getAccumulatedText();
7778
if (StringUtils.isBlank(content)) {
78-
// Nothing to title; leave the block alone (still in-progress) so a later delta can keep streaming.
79+
// Nothing to title; leave the block alone (still in-progress) so a later thinking stream fragment can keep
80+
// streaming.
7981
return;
8082
}
81-
active.sealed = true;
83+
CopilotLanguageServerConnection ls = CopilotCore.getPlugin().getCopilotLanguageServer();
84+
if (ls == null) {
85+
target.showCancelled();
86+
requestLayout();
87+
return;
88+
}
89+
target.markSealed();
8290
String[] titles = target.getExtractedTitles();
8391
// Server schema rejects null entries inside extractedTitles, so we send one of the two fields, never both.
8492
boolean hasTitles = titles.length > 0;
8593
GenerateThinkingTitleParams params = new GenerateThinkingTitleParams(hasTitles ? null : content,
8694
hasTitles ? titles : null);
87-
CopilotCore.getPlugin().getCopilotLanguageServer().generateThinkingTitle(params)
95+
ls.generateThinkingTitle(params)
8896
.thenAccept(resp -> SwtUtils.invokeOnDisplayThread(() -> {
8997
if (target.isDisposed() || target.isFinalized()) {
9098
return;

0 commit comments

Comments
 (0)