mirror of
https://github.com/cogwheel0/conduit.git
synced 2026-08-27 11:31:42 +00:00
f843481092
* Polish chat streaming timeline * Refactor streaming markdown into display parts * Keep streaming markdown part metadata local * Address review round 1 on chat streaming timeline Correctness: - chatTurnPhaseForMessage: treat metadata['responseDone'] as completed so the typing footer hides and the action row appears during the responseDone gap - timeline tail detection excludes archivedVariant assistants (history sliver already hides them; prevents a stale archived answer flashing as the live tail) - markdown display parts: image-only blocks keep non-empty normalizedContent so ConduitMarkdownWidget no longer drops standalone/base64 images - compose(): recompute mutableBlockStartIndex after tool_calls details merge across the frozen/tail boundary so the streaming tail keeps its fade; align the segment filter with isEmpty so blocks-only segments survive - bottom anchor: release the sticky latch on the final correction attempt so the scroll-to-bottom button isn't suppressed when a correction never lands - chat_page: suppress mount fade for a completed assistant migrating into history Simplification (behavior-preserving): - drop redundant streaming-preservation guards duplicated in the stale-echo check - remove dead carried state: ChatTurnFooterHost.sourceIndex/phase, MarkdownDisplayPart.kind/sourceBlockIndex/sourceBlockId, historyIndexByMessageId, completedFooterHost, and the pass-through bottom-anchor wrapper Tests: add coverage for the failed/responseDone/archived tail phases, the stale-echo completion fields + server-advance/non-tail preservation branches, the bottom-anchor state transitions + latch release, the image-block render path, display-part id uniqueness, and the tool_calls compose realignment + frozen tail. * Address review round 2 on chat streaming timeline Simplification: - extract the shared assistant-row wiring used by the history and live-tail slivers into _buildAssistantMessageRowContent, removing ~35 lines of duplication - gate animateOnMount on the settled (completed/failed) phase instead of responseDone alone, applied uniformly to both slivers — suppresses the mount fade for a completed tail on first load and on tail->history migration even when responseDone was never written, and removes the prior asymmetry - drop the redundant tailAssistantSourceIndex null check when building footerHost - collapse the unreachable non-ChatMessage fallbacks in _responseCompleted / _uiTreatsAsStreaming to the shared phase rule Tests: add coverage for the scroll-to-bottom hysteresis (currentlyShowing:true) and the non-scrollable re-anchor path; multi-row historyIndexByMessageKey; the no-mutable-metadata streaming display-part fallback; details-block display-part documents without a root node; compose() single-segment and empty-segment branches; co-firing streaming-state/content/modelName preservation; the StreamingTurnFooter running-haptic (fire-once, re-arm, suppression); and the errored-streaming action-footer phase at the widget level. * Address review round 3 on chat streaming timeline Correctness / consistency: - _activeStreamingAssistantId uses the chatTurnPhaseForMessage running phase so the scroll-keepalive agrees with the footer/pin logic across the responseDone gap - _correctStickyBottomAnchor clears the sticky latch when a correction is abandoned (widget gone / scroll not yet attached), instead of leaving it latched and wrongly suppressing the scroll-to-bottom button; generation / user-interaction aborts still bail without touching the latch Simplification: - ChatTurnFooterHost stores just the messageId (the only field ever read) - extract the duplicated sticky-latch predicate into _stickyLatchHolds - document the details-block id-rebasing invariant in _rebaseCompiledMarkdownBlock Tests: cover the sources/codeExecutions stale-echo retirement branches, the empty-message-list timeline, the completed-footer predicate across all phases, the empty-document and details-group display-part branches, and the streaming display-part cache reuse/eviction path across a shrinking part list. * Address review round 4 on chat streaming timeline Correctness / simplification: - _correctStickyBottomAnchor checks correction-generation ownership before the abandoned-correction latch clear, so a stale generation can't drop a newer correction's sticky latch - inline the unconditional footerSwitchDuration (Duration.zero) at its two AnimatedSwitcher call sites Tests: cover the trimLastBlockBottomPadding=false branch (trailing-block spacing) and its base-render cache invalidation; the StreamingTurnFooter hidden/empty state and the haptic re-arm on hide-then-reshow of the same id; the verifyStickyCorrection nearBottom-over-isFinalAttempt precedence; sticky-latch suppression while the user interacts; and display-part id dedup for blocks that share a block id. * Address review round 5 on chat streaming timeline All round-5 findings were test-coverage gaps (no code defects found). - add a @visibleForTesting debugShouldCleanupStreamingFromServer accessor and a seam test pinning the stale-echo guard vs. real (responseDone/error) completions in _shouldCleanupStreamingFromServer independently of the merge path - cover the boundToTail (socket-resumed foreign server id) streaming-state preservation path - cover shouldShowScrollToBottom with the sticky latch held AND currentlyShowing - verify the isMutableTail -> ConduitMarkdownWidget.enableStreamingTextFade gate end-to-end (only the mutable tail part fades) Not done: extracting the _correctStickyBottomAnchor generation counter into the controller purely to unit-test it. The reviewer confirmed no bug exists there and the generation-ownership ordering is already covered by the round-4 guard fix; a test-only production refactor was judged not worthwhile. * Lock chatTurnPhaseForMessage null / non-assistant branch Final convergence pass: rounds 5 and 6 surfaced no code defects, only test-coverage observations. Add a direct test for the one remaining trivial untested public-function branch (null / non-assistant -> ChatTurnPhase.none). * Fix bottom-anchor latch detach + cross-conversation leak (review) Two Medium findings from Macroscope on the bottom-anchor logic: - updateAnchor() no longer force-detaches mid-drag: while a sticky content change is pending it keeps the view anchored regardless of isUserInteractingWithScroll, so only shouldDetachForUserScrollAway (which honors userScrollAwayThreshold) breaks the latch. Previously a sub-threshold accidental drag during streaming dropped bottom anchoring. - _handleConversationChanged() clears the sticky latch on conversation switch so a new conversation doesn't inherit a stale anchored state. The clear lives here rather than in _cancelPendingStickyBottomCorrection() because that method is also called on drag-start, where the latch must survive to gate the scroll-away threshold. Rewrote the interaction controller test to lock the corrected small-drag-keeps- anchor / threshold-drag-detaches behavior. * Fix responseDone/failed phase staleness (CodeRabbit review) - Include responseDone in the chat message structure signature so the list shell rebuilds on the responseDone transition (running footer host / pin-to-top would otherwise stay stale through the responseDone gap until the transport isStreaming flag cleared). - AssistantMessageWidget: re-run the action-row settle when message.error changes, and settle on the failed phase, so a turn that fails in place while isStreaming is still true reveals the action row instead of staying stuck. Added a mid-stream-fail regression widget test. - CompiledMarkdownDocument.isMutableRootBlock bounds-checks the index against blocks.length so an out-of-range query can't be misclassified as mutable. Not changed: CodeRabbit's suggestion to clear the latch in _cancelPendingStickyBottomCorrection (would break the drag-start threshold; the conversation-change leak is already handled in 2d4a08a). * Smooth streaming bottom-follow + fix long-response scroll jump Two streaming/scroll UX issues (root-caused with adversarial verification): Bug 1 — streamed text stepped UP from the bottom instead of fading in place: bottom-stickiness was a per-chunk instant jumpTo (_scrollToBottom(smooth:false)), so each token batch yanked the viewport up a frame after the taller content was painted, overpowering the in-place suffix fade. _handleLiveTurnSizeChange now requests a delta-aware smooth follow: for small per-chunk growth (<= 48px from bottom) it glides to the bottom over 140ms and settles the sticky latch directly (verifyStickyCorrection(nearBottom:true)) without the synchronous-distance read or per-frame recursion the jump path uses; larger growth and all non-live corrections (initial settle, extent-invalidation) still jumpTo. The latch/verify logic and the _isUserInteractingWithScroll guard are otherwise untouched. Bug 2 — scrolling up through a long response jumped past the prompt to the first response: _estimateChatMessageExtent hard-clamped to 2400px, far below a real multi-thousand-pixel response, so a never-yet-measured long history row (e.g. one that migrated out of the live-tail sliver) produced a large SuperSliverList scroll-offset correction on first reveal. Raised the ceiling to 20000, added code-block/image structure terms, and excluded base64 data-uri payloads from the line estimate so a generated image can't over-estimate. Added estimate tests. Note: Bug 1 is animation behavior (not unit-testable); verify in-app with a fast streaming response that text fades in place, the scroll-to-bottom button never flashes mid-stream, and stickiness still converges. * Estimate rendered height for raw standalone base64 image lines (review) Macroscope: the row-extent estimate stripped data-uri payloads from the line count but only added a per-image term for markdown ![] images, so a raw standalone base64 image line (the common generated-image form, converted to an image at display time with no markdown wrapper) under-estimated to ~one line and re-introduced the scroll jump. Now count each rendered image once: markdown ![] images plus raw standalone data-uri lines (a data-uri inside a markdown image is not at line start, so it isn't double-counted). Added a regression test. * Use idiomatic string generation in estimate tests (review) CodeRabbit flagged 'A' * 20000. It compiled/ran via a transitively-imported String operator* extension, but that's a fragile dependency; switch to List.filled(20000, 'A').join() for an unambiguous, self-contained payload. * Exclude fenced-code-block markup from the image extent heuristic (review) Macroscope: image/data-uri markup inside a fenced code block renders verbatim (as text), so counting it as a rendered image (+220px) and stripping its data-uri payload from the line count corrupted the estimate — a code sample displaying a base64 blob would under-estimate and re-introduce the scroll jump. Now separate fenced code blocks: count their content in full for line height and apply the image-term / data-uri-strip logic only to prose outside them. Added a test. * Harden #540 fix: clear stale content on scope change + unique scopeless PageStorage keys (review) Two Macroscope findings on the #540 fix area: - Cross-message/version stale content: with 'always render a non-null doc', a StreamingMarkdownWidget reused across a stateScopeId change (a version/message switch) briefly showed the previous scope's content while the new body compiled. Fix without re-introducing the #540 flash: thread a clearStaleDocument signal so a scope change clears the stale doc (skeleton covers the gap), while same-scope content growth keeps its document. Added a regression test (fails before: stale 'First version' lingers on scope switch). - PageStorage key collision: _stateScopeIdForPart fell back to the raw partId when no stateScopeId was given, and block ids repeat across documents, so two scope-less StreamingMarkdownWidgets on a route cross-restored details expansion state. Use a unique per-instance scopeless fallback prefix. * Retire stale streaming echo once server moves past the turn (#537 review) Greptile: _shouldCleanupStreamingFromServer treated an empty non-streaming server echo of the in-flight assistant as 'keep streaming' even when the server snapshot also carried newer messages after it. The stale-echo early return ran before the serverMessages.length > state.length cleanup check, so the old assistant stayed marked as the active stream after the server had already moved past that turn — leaving the streaming footer/task state pinned to a no-longer-tail message. Gate the stale-echo return on the server NOT having additional messages, the same serverHasAdditionalMessages guard the sibling _shouldPreserveLocalAssistantStreamingState already uses, so the cleanup and preserve paths agree. Adds a regression case to the existing cleanup test. * Finalize the stream when a metadata-only server snapshot arrives (#537 review) _isStaleStreamingAssistantEcho classified any content-empty server snapshot as a stale streaming echo, ignoring statusHistory, versions, and usage. A final metadata-only snapshot (e.g. a closing statusHistory entry with empty content) was therefore misclassified as stale, which forced isStreaming back to true via _preserveFreshLocalAssistantState and kept _shouldCleanupStreamingFromServer from finalizing — leaving the typing indicator stuck until a later refresh. Require statusHistory/versions/usage to also be empty before treating a snapshot as a stale echo, and extend the parameterized completion-field seam test with statusHistory, versions, and usage cases. * Keep the stream alive for metadata-only status echoes (#537 review) Reverts the statusHistory/versions/usage check added in de7da9b1. Those fields are populated on the assistant message *during* streaming — the server pushes status/usage updates as content-empty, non-streaming snapshots before the answer tokens arrive (streaming_helper) — so treating their presence as a completed update retired the active stream prematurely and dropped the typing footer mid-turn (Greptile P1). Real completion is already proven by responseDone/error or by non-empty content/output/files/embeds/followUps/sources/codeExecutions, so a finished turn is never a metadata-only echo. Document why those fields are excluded and replace the completion-field test cases with a regression test that a content-empty, in-progress status-only echo keeps the stream alive. * Preserve resume-bound remote id through stale server adopts (#537 review) Dropping transport before preserve cleared `_boundRemoteMessageId`, so a foreign-id stale echo could retire the streaming tail early. Also skip DB-watch adopts while resume streaming is active. * Cancel the message stream before dropping transport on adopt cleanup CodeRabbit: `_dropStreamingTransportState` nulls `_messageStream` without canceling the controller, so cancel must run first while it is still attached. * Address post-rebase review findings on streaming timeline polish Fix blank details shrink, sticky detach on scroll-toward-bottom, queued typing footer, pin-to-top stuck after drag, and foreign-id stream cleanup. * Add staggered follow-up animation and fix pin-to-top scroll jump * fix(chat): preserve streaming anchor on drag start Non-update scroll notifications were mapped to a synthetic threshold-sized delta after the rebase, bypassing the small-drag guard. Pass no delta until a real ScrollUpdate arrives and cover that boundary. Also remove the stale-scope test duplicated from main so the shared markdown cache no longer makes the full suite fail. * fix(chat): detach streaming anchor for pointer scrolls Wheel and trackpad updates carry no drag details, so the touch-only gate skipped their delta after the directional notification. Treat null-detail updates as user-driven only while a user interaction is active, preserving programmatic scroll corrections. Add classifier coverage for touch, pointer, and programmatic paths so this input-path regression cannot recur unnoticed. * fix(chat): cancel retired streaming controllers Server adoption could release a tracked StreamingResponseController without cancelling its subscription, allowing late chunks or terminal callbacks to run against an adopted snapshot. Make the shared stale-transport teardown cancel the controller before dropping its reference. Add provider coverage that adopts a replacement snapshot and proves the retired controller is inactive and ignores a late chunk. * test(chat): cover streaming cleanup cancellation Exercise server adoption with a live streaming tail and a real tracked controller so the needsCleanup path is guarded directly. Assert responseDone settles the message, cancels the controller, and suppresses late chunks. * fix(chat): remove post-response settle motion Completion reused two independently added motion paths: follow-up suggestions ran a staggered entrance while every measured live-tail resize requested the streaming smooth bottom-follow. That chained a settle animation with an animated scroll after the response. Render follow-ups immediately and allow smooth bottom correction only while the turn phase is running. responseDone and settled follow-up layout changes now preserve the bottom anchor instantly, with regressions covering both decisions. * fix(chat): polish motion and reduced motion * fix(chat): address motion review feedback * fix(chat): keep archived versions settled |
||
|---|---|---|
| .. | ||
| app_theme.dart | ||
| color_tokens.dart | ||
| conduit_button_styles.dart | ||
| conduit_input_styles.dart | ||
| theme_extensions.dart | ||
| tweakcn_themes.dart | ||