fix: restore MSC4357 streaming animation for bot replies#99
Open
TigerInYourDream wants to merge 9 commits intomainfrom
Open
fix: restore MSC4357 streaming animation for bot replies#99TigerInYourDream wants to merge 9 commits intomainfrom
TigerInYourDream wants to merge 9 commits intomainfrom
Conversation
Adopt Moly-style live snapshot rendering: display the latest full text with a trailing cursor during live updates, then fall through to the existing rich markdown renderer once the `live` field clears. Regression introduced by 1a33998: `should_render_streaming_full_snapshot` disabled the PR #14 typewriter for any bot reply with HTML formatted_body or markdown syntax, i.e. virtually every AI response. The previous client-side fixed-cadence pacer also produced long tails or catch-up bursts depending on network timing. - streaming_animation.rs: drop render_full_target and local reveal cadence; new/update/restore sync displayed state to the latest target; tick degrades to a no-op - room_screen.rs: thread streaming plaintext + cursor into the bot-card populate path; fix clear_cache and same-event_id scan_range paths that previously dropped new live streams - link_preview.rs: reset collapsible buttons when preview count shrinks (stale state leak in recycled PortalList items) - specs/task-restore-streaming-animation.spec.md: task contract External dependency: if the appservice never emits the MSC4357 finish_stream marker, robrix2 waits the existing 5-minute live-stall timeout before restoring rich markdown. This PR does not modify any appservice. cargo check clean; streaming_animation 15/15; link_preview 3/3.
Author
|
Added bounded per-update tween (TWEEN_DURATION = 150ms, strictly < any reasonable server edit throttle) so live streaming feels smooth even on coarse-grained server updates. Three additional commits on this branch:
Spec updated in this same branch with the tween decisions and acceptance criteria. Plan: Verification:
The tween is intentionally short (150ms) so it always completes before the next server edit (Octos throttles edits to ≥200ms). This guarantees no PR #14-style long tail while smoothing the visual stutter from coarse server pacing. |
Spec adds the bounded-tween contract to the 决策 section (TWEEN_DURATION = 150ms, growth-defers-sync, finish-syncs-immediately, linear interpolation in tick_with_elapsed, needs_frame reflects tween progress) and replaces the obsolete tick-no-op scenario with five new acceptance scenarios that map 1:1 to the unit tests. Plan documents the TDD task breakdown that produced the three preceding commits.
restore() previously called Self::new(new_text, is_live), which invokes sync_displayed_to_target() and sets displayed == target. That cancelled any active tween whenever the timeline did a clear_cache or full_snapshot rebuild mid-stream — the visible artefact was "animation stuck for a beat, then text appears all at once". Preserve the previous tween state (displayed_char_count, displayed_byte_offset, reveal_base_char, reveal_base_time) when previous was mid-tween (displayed < target) and the new update is still live. Clamp displayed and reveal_base_char to the new target's char count and recompute byte offset so slicing in advance_displayed stays safe. Added test_restore_preserves_in_flight_tween covering the clear_cache / full_snapshot rebuild case. cargo test --lib home::streaming_animation now 21/21.
The 意图 section previously said "不再做本地二次 cadence 动画" while the 决策 section introduced TWEEN_DURATION = 150ms and bounded per-update tween — a real internal contradiction that would mislead future implementers. Rewrite 意图 to state both goals explicitly (align with server pace to avoid PR #14 long tail, AND smooth out Matrix edit coarseness with a bounded local tween) and explain the Moly vs Robrix architectural difference (openclaw SSE token rate ~30-50ms vs Matrix edit throttle 200-1000ms) that motivates the tween. agent-spec lint stays at 100%.
Previous restore() preserved reveal_base_time across rebuilds unconditionally. When the rebuild brought a longer target than the previous tween was heading toward, the next tick() would pair old elapsed with a larger delta and jump several chars at once — just re-introducing the "stuck then jump" artefact via a different path. Split the preserve branch on target size: - target unchanged or shrunk: keep old reveal_base_time (tween continues seamlessly, visual continuity across rebuild) - target grew: open a fresh tween window (reveal_base_char = current displayed, reveal_base_time = now) so the next tick starts a new 150ms interpolation from where the user is looking Added test_restore_growth_rebuild_opens_fresh_tween_window to pin the growth-rebuild behaviour at the unit level. Updated the integration regression at room_screen.rs test_full_snapshot_rebuild_restores_live_cached_streams to assert the tween-preserve semantics end-to-end — previously it still verified the obsolete "snap to fully revealed" contract, leaving the preserve path uncovered at the integration layer. cargo test --lib home::streaming_animation: 22/22 cargo test --lib test_full_snapshot_rebuild_: 3/3 cargo test --lib test_clear_cache_update_rebuild_: 3/3 Full lib: 253 pass / same 4 pre-existing failures, no new regressions.
restore() previously preserved the old reveal_base_time for any non-growth rebuild. That meant a same-length rewrite or shrink during a clear_cache / full_snapshot rebuild would keep animating a stale prefix against the new text — inconsistent with update_target()'s non-growth branch which syncs immediately. Split the preserve branch into three cases: - Same text (rebuild carried no new content): preserve tween in place (visual continuity for clear_cache/full_snapshot replays). - Different text + target grew: open a fresh tween window from current displayed position (existing behaviour). - Different text + same length or shrunk: sync to target, matching update_target()'s non-growth branch and avoiding stale-prefix rendering. Added unit test_restore_non_growth_rewrite_syncs_to_target to pin the rewrite-to-sync behaviour. Added integration regression test_clear_cache_update_rebuild_preserves_in_flight_tween_from_cached_state in room_screen so the tween-preserve guarantee now rides through rebuild_streaming_messages_for_clear_cache_update end-to-end, not just restore() in isolation. Previously only the full_snapshot path was covered at the integration layer. cargo test --lib home::streaming_animation: 23/23 cargo test --lib test_full_snapshot_rebuild_: 3/3 cargo test --lib test_clear_cache_update_rebuild_: 4/4 Full lib: 255 pass / same 4 pre-existing failures, no new regressions.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
livefield clearsshould_render_streaming_full_snapshotdisabled the PR feat: MSC4357 streaming typewriter animation for bot responses #14 typewriter for any bot reply with HTMLformatted_bodyor markdown syntax (= virtually every AI response)clear_cache=truebatches on fresh room subscriptions, andsame-event_idtext growth in the non-clear_cache scan rangeDesign
See
specs/task-restore-streaming-animation.spec.mdfor the full task contract. Key decisions:StreamingAnimStatestays as the MSC4357 lifecycle tracker but no longer paces local character reveal;new/update_target/restoresyncdisplayed_*to the latest full targetneeds_frame()returns false during live; the existing frame handler only runs a cleanup pass whenis_complete()flips true on the finish editbot_card_markdown,bot_card_markdown_plain,bot_status_strip,bot_metadata_footer, andlink_preview_viewto keep recycled PortalList items from bleeding state into streaming repliesExternal dependency
If the appservice never emits the MSC4357
finish_streamsignal (i.e. the terminal edit still carries thelivefield), robrix2 waits the existing 5-minute live-stall timeout before restoring rich markdown. This PR does not modify Octos or any appservice. Recommended follow-up: upstream Octos PR toedit_message→finish_streamon terminal streaming edits.Test plan
cargo check --libcleancargo test --lib home::streaming_animation15/15 passcargo test --lib home::link_preview3/3 pass