perf(chat): improve WebChat stability and streaming performance #362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Throttle streaming DOM upserts more aggressively and process DOM updates in smaller batches to keep the UI responsive while moving/resizing the WebChat dialog.
Move chat message HTML/markdown rendering off the UI thread (including tool result ordering inserts) and ensure only the latest render per DOM key is applied.
…ug logging
- Add keyed DOM update queue (`_keyedDomUpdateLatest` + `_keyedDomUpdateQueue`) to ensure only latest render per DOM key is applied, preventing redundant updates during streaming
- Wrap all `Debug.WriteLine` calls in `[Conditional("DEBUG")]` `DebugLog` method to eliminate debug overhead in release builds
- Move HTML equality checks inside `#if DEBUG` blocks to skip expensive string comparisons in production
…fing, and wipe-in animations - Add template cache (`_templateCache`) and LRU HTML cache (`_htmlLru`) to avoid redundant DOM parsing and equality checks during streaming - Implement queued DOM operations (`enqueueDomOp` + `flushDomOps`) batched via `requestAnimationFrame` to reduce layout thrashing - Add sampled equality diffing (25% sample rate) via `shouldSkipBecauseSame` to skip redundant renders when HTML unchanged
…reservation - Remove unused `stream-update` CSS animations and `addStreamUpdateAnimation` JS function - Remove unused `MAX_CONCURRENT_SCRIPTS` and `_activeScripts` variables from chat-script.js - Add `FlushPendingTextStateForTurn` to ensure throttled text deltas are rendered before tool calls appear in the UI - Fix `ConversationSession` to preserve `ContextFilter` when rebuilding AIBody in special turns and helper methods
… improve token efficiency - Add new `instruction_get` tool that returns detailed operational instructions on-demand for canvas operations, component discovery, scripting workflows, and knowledge base usage - Refactor `CanvasButton` system prompt to be concise and delegate detailed tool usage guidelines to `instruction_get` calls - Remove verbose inline documentation for tool workflows (canvas state reading, component discovery, scripting, knowledge base)
…meter validation - Enhance `instruction_get` tool description to explicitly mention required `topic` argument since some models (MistralAI, OpenAI) don't always respect JSON Schema `required` fields but do follow description text - Add explicit validation for missing/empty required parameters in `ToolJsonSchemaValidator` with actionable error messages prompting retry - Remove redundant null check in `instruction_get` tool body
…adapter discovery - Extract duplicated streaming delta processing (~80 lines) into shared `ProcessStreamingDeltasAsync` helper in `ConversationSession` - Add `GetStreamingAdapter()` to `IAIProvider` interface with caching in `AIProvider` base class, replacing reflection-based discovery - Add `CreateStreamingAdapter()` virtual method for providers to override; update OpenAI, DeepSeek, MistralAI, Anthropic, and OpenRouter - Removed "Clear" button from webchat and replaced with a "Regen" button on debug
Contributor
|
🏷️ This PR has been automatically assigned to milestone 1.2.2-alpha based on the version in |
marc-romu
added
component: AI Chat
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves WebChat stability and streaming performance through comprehensive optimizations addressing UI freezes during streaming (issue #261). Key improvements include:
- Refactored streaming infrastructure with cached adapter discovery and shared delta processing
- Enhanced UI responsiveness through DOM batching, throttling, and render optimizations
- Added
instruction_gettool to reduce system prompt token usage via on-demand guidance retrieval
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
src/SmartHopper.Providers.*/ |
Refactored streaming adapters from public reflection-based methods to protected virtual factory pattern |
src/SmartHopper.Infrastructure/AIProviders/AIProvider.cs |
Added cached streaming adapter support with GetStreamingAdapter() and CreateStreamingAdapter() |
src/SmartHopper.Infrastructure/AICall/Sessions/ConversationSession.cs |
Extracted shared ProcessStreamingDeltasAsync() helper reducing ~80 lines of duplication |
src/SmartHopper.Core/UI/Chat/WebChatDialog.cs |
Implemented DOM operation batching, render versioning, and move/resize throttling to prevent UI freezes |
src/SmartHopper.Core/UI/Chat/WebChatObserver.cs |
Refactored state management with TurnRenderState and SegmentState for cleaner turn tracking |
src/SmartHopper.Core/UI/Chat/Resources/js/chat-script.js |
Added template caching, LRU diffing, queued DOM ops, and wipe-in animations for performance |
src/SmartHopper.Core.Grasshopper/AITools/instruction_get.cs |
New tool providing on-demand operational instructions to reduce system prompt size |
src/SmartHopper.Core/AIContext/FileContextProvider.cs |
Fixed selection context by reading state on UI thread with ManualResetEventSlim synchronization |
src/SmartHopper.Infrastructure/AICall/Validation/ToolJsonSchemaValidator.cs |
Enhanced validation to explicitly check required parameters and provide actionable error messages |
Solution.props, CHANGELOG.md, README.md |
Version bump to 1.2.2-dev.251224 with comprehensive changelog entries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| fileName = Path.GetFileName(path); | ||
| // Wait for UI thread to complete (timeout after 5 seconds to avoid deadlock) | ||
| uiThreadComplete.Wait(TimeSpan.FromSeconds(5)); |
There was a problem hiding this comment.
The wait timeout is hardcoded to 5 seconds. If the UI thread is heavily loaded or blocked, this could cause context information to be incomplete. Consider making this timeout configurable or at least documenting why 5 seconds was chosen as the threshold.
Comment on lines
+108
to
+116
| function addWipeAnimation(node) { | ||
| try { | ||
| if (!node || !node.classList) return; | ||
| node.classList.add('wipe-in'); | ||
| node.style.setProperty('--wipe-duration', `${RENDER_ANIM_DURATION_MS}ms`); | ||
| const remove = () => node.classList.remove('wipe-in'); | ||
| node.addEventListener('animationend', remove, { once: true }); | ||
| } catch { /* ignore */ } | ||
| } |
There was a problem hiding this comment.
The comment says this function adds a wipe-in animation, but it actually removes it via the 'animationend' listener. Consider clarifying that this function both adds and schedules removal of the animation class.
| { | ||
| yield return new AITool( | ||
| name: ToolName, | ||
| description: "Returns detailed operational instructions for SmartHopper. REQUIRED: Pass `topic` with one of: canvas, discovery, scripting, knowledge, ghjson, selected, errors, locks, visibility. Use this to retrieve guidance instead of relying on a long system prompt.", |
There was a problem hiding this comment.
The comment mentions "REQUIRED: Pass topic" but JSON Schema already declares it as required. While this redundancy may help some models, consider that the description is quite verbose. If models consistently ignore schema requirements, this might indicate a broader validation issue to address upstream.
Suggested change
| description: "Returns detailed operational instructions for SmartHopper. REQUIRED: Pass `topic` with one of: canvas, discovery, scripting, knowledge, ghjson, selected, errors, locks, visibility. Use this to retrieve guidance instead of relying on a long system prompt.", | |
| description: "Returns detailed operational instructions for SmartHopper based on the requested topic. Valid topics: canvas, discovery, scripting, knowledge, ghjson, selected, errors, locks, visibility. Use this to retrieve guidance instead of relying on a long system prompt.", |
| <Project> | ||
| <PropertyGroup> | ||
| <SolutionVersion>1.2.1-alpha</SolutionVersion> | ||
| <SolutionVersion>1.2.2-dev.251224</SolutionVersion> |
There was a problem hiding this comment.
The version format "1.2.2-dev.251224" appears non-standard for semantic versioning. According to SemVer, pre-release identifiers should use hyphens, not dots between segments (e.g., "1.2.2-dev-251224" or "1.2.2-dev+251224" for build metadata). This could cause issues with version parsing tools.
Suggested change
| <SolutionVersion>1.2.2-dev.251224</SolutionVersion> | |
| <SolutionVersion>1.2.2-dev-251224</SolutionVersion> |
| --> | ||
| <PropertyGroup> | ||
| <SmartHopperPublicKey>This value is automatically replaced by the build tooling before official builds.</SmartHopperPublicKey> | ||
| <SmartHopperPublicKey>0024000004800000940000000602000000240000525341310004000001000100b90ff13176f06b3385ce4bafee2a5177994228e8726e444377056f2ff11813457d594162f7542e7621eedec5445ce0e079e7d01357cf2463fb73aa5e248a34e57fe1999daa6a17f493bdafc5cfdd4cd80d14cb00326ba745a862a3cd5686504d2ae9e6e06e9f4ccebd2bffd7b990e617f6ad8a42397a20123fb373ce582085cc</SmartHopperPublicKey> |
There was a problem hiding this comment.
The PublicKey value is being hardcoded directly in the project file. The comment states this is "automatically replaced by build tooling before official builds", but having a real key visible here (even temporarily) could be a security concern. Consider using a placeholder value that's clearly not a valid key.
Comment on lines
233
to
238
| if (string.IsNullOrWhiteSpace(followKey)) | ||
| { | ||
| #if DEBUG | ||
| DebugLog($"[WebChatDialog] UpsertMessageAfter WARNING: followKey is null/empty for key={domKey}, will fallback to normal upsert"); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
If-statement with an empty then-branch and no else-branch.
Suggested change
| if (string.IsNullOrWhiteSpace(followKey)) | |
| { | |
| #if DEBUG | |
| DebugLog($"[WebChatDialog] UpsertMessageAfter WARNING: followKey is null/empty for key={domKey}, will fallback to normal upsert"); | |
| #endif | |
| } | |
| #if DEBUG | |
| if (string.IsNullOrWhiteSpace(followKey)) | |
| { | |
| DebugLog($"[WebChatDialog] UpsertMessageAfter WARNING: followKey is null/empty for key={domKey}, will fallback to normal upsert"); | |
| } | |
| #endif |
| haveStreamedAny = true; | ||
| } | ||
|
|
||
| toolCallsEmitted = true; |
There was a problem hiding this comment.
This assignment to toolCallsEmitted is useless, since its value is never read.
Comment on lines
946
to
952
| if (kv.Value?.Aggregated is AIInteractionText aggregatedText && HasRenderableText(aggregatedText)) | ||
| { | ||
| if (this.ShouldRenderDelta(streamKey, aggregatedText)) | ||
| { | ||
| DebugLog($"[WebChatObserver] FlushPendingTextStateForTurn: flushing streamKey={streamKey}"); | ||
| this._dialog.UpsertMessageByKey(streamKey, aggregatedText, source: "FlushPendingText"); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (kv.Value?.Aggregated is AIInteractionText aggregatedText && HasRenderableText(aggregatedText)) | |
| { | |
| if (this.ShouldRenderDelta(streamKey, aggregatedText)) | |
| { | |
| DebugLog($"[WebChatObserver] FlushPendingTextStateForTurn: flushing streamKey={streamKey}"); | |
| this._dialog.UpsertMessageByKey(streamKey, aggregatedText, source: "FlushPendingText"); | |
| } | |
| if (kv.Value?.Aggregated is AIInteractionText aggregatedText | |
| && HasRenderableText(aggregatedText) | |
| && this.ShouldRenderDelta(streamKey, aggregatedText)) | |
| { | |
| DebugLog($"[WebChatObserver] FlushPendingTextStateForTurn: flushing streamKey={streamKey}"); | |
| this._dialog.UpsertMessageByKey(streamKey, aggregatedText, source: "FlushPendingText"); |
| // Tracks per-turn text segments so multiple text messages in a single turn | ||
| // are rendered as distinct bubbles. Keys are the base stream key (e.g., "turn:{TurnId}:{agent}"). | ||
| private readonly Dictionary<string, int> _textInteractionSegments = new Dictionary<string, int>(StringComparer.Ordinal); | ||
| private DateTime _lastDeltaLogUtc = DateTime.MinValue; |
There was a problem hiding this comment.
Field '_lastDeltaLogUtc' can be 'readonly'.
Comment on lines
+81
to
88
| if (useStreaming) | ||
| { | ||
| result = await this.ExecuteStreamingSpecialTurnAsync(specialRequest, config, turnId, effectiveCt).ConfigureAwait(false); | ||
| } | ||
| finally | ||
| else | ||
| { | ||
| linkedCts?.Dispose(); | ||
| result = await this.ExecuteNonStreamingSpecialTurnAsync(specialRequest, config, turnId, effectiveCt).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR improves WebChat stability and streaming performance through several optimizations:
ProcessStreamingDeltasAsynchelper method and optimized provider adapter discovery with caching instead of reflectioninstruction_gettool to reduce system prompt size and improved its reliability with explicit parameter validationMitigates #261.
Breaking Changes
No breaking changes.
Testing Done
instruction_gettool functionality across different AI providersChecklist