feat(interactive-card): client fallback to finalize stuck progress cards (WS-99) - #1035
feat(interactive-card): client fallback to finalize stuck progress cards (WS-99)#1035pkuWMH wants to merge 2 commits into
Conversation
When an assistant sends a type=1 final text but its most recent type=17 agent-progress card is still non-terminal (header starts with 🤖/⏳) and has been idle for >=3s since its last frame, downgrade that card in the render layer to "completed (no terminal update received)": swap the running-step ⏳ for⚠️ , dim the in-progress visuals, and show a "client fallback" tag. Matches strictly by senderId so concurrent assistants are never mis-closed, and only overlays local UI state without mutating message content. Co-authored-by: multica-agent <github@multica.ai>
Jerry-Xin
left a comment
There was a problem hiding this comment.
Client-side fallback that finalizes stuck agent_progress_v1 (type=17) cards when the sender omits the terminal frame. Detection is conservative and display-only (no message/server mutation), but the local fallback flag is never cleared when a genuine terminal frame later arrives — leaving a "no explicit terminal" banner stuck on a legitimately-completed card.
🔴 Blocking
Sticky fallback banner on late-arriving genuine terminal frame — the reconcile the PR is meant to handle is not fully closed.
packages/dmworkbase/src/Components/Conversation/vm.ts:1491setsm.localFallbackApplied = trueand it is never reset (only set-once; the only other reference atvm.ts:1484is the idempotency early-return).- When the real server terminal frame arrives later via
updateMessageByMessageExtras(vm.ts:1504), it updatesremoteExtraand re-stampsprogressUpdatedAtSecbut does not clearlocalFallbackApplied. - The terminal frame keeps
metadata.octo_layout === "agent_progress_v1", soisAgentProgressCardstays true.InteractiveCardCell.isFallbackFinalized=localFallbackApplied && isAgentProgressCard(card)therefore stays true. - Result: after the server's real "✅ 已完成" frame renders, the
wk-interactive-card-fallbackbanner ("⚠️ 已完成(未收到显式终态)" + "Client fallback" tag) remains displayed on a card that genuinely reached its real terminal state. This is exactly the client-fallback vs late-server-terminal race, and here server does NOT fully win the annotation. - Fix: when a frame refresh makes the card terminal (header hits a terminal marker) or on any
updateMessageByMessageExtrasre-stamp for that card, clearlocalFallbackAppliedso the fallback annotation is retracted once a real terminal frame arrives.
💬 Non-blocking
vm.ts:1489idle guard readsprogressUpdatedAtSec, stamped only viastampProgressCardArrivalon WS arrival / extras update. For a card loaded from history (first-page load, no live arrival stamp),progressUpdatedAtSecis undefined andisProgressCardIdleEnough(undefined, ...)returns false — so a card that was already stuck before the session opened will not be finalized by a subsequently-received final text. Acceptable/conservative, but worth a comment noting history-loaded stuck cards are out of scope.- Consider adding one test that asserts the flag is cleared (or the banner retracted) once a terminal frame arrives, to lock the reconcile behavior above.
✅ Highlights
- Display-only by design: only a local
MessageWrapflag (Model.tsx) is set; card tree and server content are untouched — no forged terminal state is sent back to server or other clients (trust/data-integrity: clean). - Conservative triggering: strict
senderIdmatch on the sender's most-recent type=17 card, terminal/waiting markers (✅⚠️ ❌ ⏹ ⏱ ⏸) never trigger,⏸sessions_yield correctly excluded, unrecognized headers do not trigger, plus a 3s idle guard against fast action sequences. - No spinner/flash regression: the fallback path sets a flag + rebuilds; the render key includes card JSON and an
fb:marker so it re-mounts at most once and never toggles a global loading/full-screen Spin state. - XSS-safe: header text is read as plain strings and the DOM overlay only rewrites text
nodeValue(⏳→⚠️ ); nodangerouslySetInnerHTML/ innerHTML sink. - Good idempotency (
vm.ts:1484) and solid unit + jsdom DOM coverage of header extraction, terminal/non-terminal/idle logic, and the no-op-without-timeline path.
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Reviewer: Octo-Q (automated review)
Head SHA: ecdca31
Code Review — PR #1035 (octo-web)
Summary
This PR adds a client-side fallback to finalize stuck agent progress cards (type=17) when the sending framework fails to emit a terminal frame. When a type=1 (final text) message arrives from the same assistant, the client checks whether the most recent progress card is still in a non-terminal state and has been idle long enough, then downgrades its display to "Completed (no terminal update received)" with visual de-emphasis. The implementation spans pure-judgment helpers (agentProgressFallback.ts), ConversationVM integration, SDK DOM overlay, CSS, i18n, and unit tests.
The design is conservative and well-guarded: every edge (null header, unknown emoji, non-progress card, self-sent text, streaming chunk) falls through to "no fallback" rather than risk a false positive. The fallback is strictly local UI state — it never writes back to message content or server.
Verification
- ✅ Data flow: VM→Cell agreement — both
resolveProgressCardinvm.tsandisFallbackFinalizedinInteractiveCardCell.tsxcallresolveEffectiveCardContent()thenisAgentProgressCard()on the sameMessageWrap, so they always agree on which card qualifies. - ✅ Rendered key invalidation — the
"fb:"prefix insyncSdkCard()(InteractiveCardCell.tsx:190) correctly forces a full SDK re-render when fallback state changes, preventing stale DOM. - ✅ No race between React render and SDK DOM —
componentDidUpdate()→syncSdkCard()→renderOctoCard()→enhanceRenderedOctoCard()is synchronous;applyAgentProgressFallbackVisualruns on the freshly mounted SDK DOM. - ✅ Idempotency —
localFallbackAppliedflag atvm.ts:1484prevents repeated fallback application; thereturnat:1498ensures at most one card per invocation. - ✅ Real terminal update compatibility —
updateMessageByMessageExtrasoverwritesremoteExtraindependently oflocalFallbackApplied, so a later server-side terminal frame renders correctly through the normal path. - ✅
streamNostreaming guard — uses the same canonical field as the message listener atvm.ts:892; consistent streaming-chunk detection. - ✅ Input-shape defensiveness —
getProgressCardHeaderTextreturnsnullfor every structural anomaly (missing body, no ColumnSet, empty columns/items);isNonTerminalProgressCardreturnsfalsefor unrecognized headers;isProgressCardIdleEnoughreturnsfalsefor non-numeric timestamps. - ✅ i18n parity — both
en-US.json:982-983andzh-CN.json:982-983add the same two keys (fallbackFinalized,fallbackTag).
Static analysis only at head ecdca3133e; build and tests not executed in this environment.
Findings
No P0/P1 issues. Three P2 items below.
P2 — Dead CSS class on timeline container (packages/dmworkbase/src/Messages/InteractiveCard/sdk/agentProgressFallback.ts:10)
wk-interactive-card-progress--fallback is added to #timeline_detail via classList.add() at :15 and the header comment says it enables style de-emphasis of in-progress visuals. However, no CSS rule for this class exists in index.css or anywhere else in the codebase. The outer wrapper opacity change works via the separate wk-interactive-card-sdk--fallback-finalized class applied by InteractiveCardCell.tsx:511, so the user-visible effect is partial — the mount container fades to 0.92 opacity, but the timeline interior (spinners, step glyphs) gets no additional visual treatment. Either add the intended CSS rule to suppress spinner animations or further de-emphasize the timeline, or remove the classList.add call and the AGENT_PROGRESS_FALLBACK_CLASS export if no inner styling is planned.
Diff-scope: new (introduced by this PR).
P2 — progressUpdatedAtSec lost on reconnect or history reload (packages/dmworkbase/src/Components/Conversation/vm.ts:1459)
stampProgressCardArrival is called only on real-time message arrival (:919) and extras-patch updates (:1514). History-load paths (loadMessages at :~1862, pulldownMessages at :~2163, pullupMessages at :~2206) create fresh MessageWrap instances via toMessageWraps() without stamping. If a reconnect or page reload occurs between a progress card's last frame and the arrival of a final text, progressUpdatedAtSec will be undefined on the new MessageWrap, isProgressCardIdleEnough returns false, and the fallback silently does not fire. The card remains visually stuck until the server sends an explicit terminal frame. This is likely acceptable given the server-side fix (WS-97 Sub-1) is the authoritative path, but consider either stamping from a server-provided timestamp during history load or documenting this as a known scope limitation.
Diff-scope: new (this PR introduces the stamping mechanism; the gap is in the new code's coverage, not pre-existing behavior).
P2 — Only the most recent stuck card per sender receives fallback (packages/dmworkbase/src/Components/Conversation/vm.ts:1479)
The backward scan through messagesOfOrigin stops at the first type=17 card from the matching sender and returns unconditionally after processing it (:1498). If an assistant emitted multiple progress cards and more than one is stuck in a non-terminal state, only the most recent one gets the fallback visual. Earlier stuck cards remain indefinitely in their "processing" display. Each subsequent final-text message from the same sender would clear at most one card. If multi-card stuck scenarios are realistic, consider continuing the scan past already-fallback or already-terminal cards to check earlier ones. If single-card scope is intentional, a brief inline comment would prevent future confusion.
Diff-scope: new (introduced by this PR; the single-card-per-scan design is a choice in the new code).
Data Flow Backtracking
| Consumed data | Upstream source | Verified flow |
|---|---|---|
progressUpdatedAtSec |
stampProgressCardArrival sets Date.now()/1000 on MessageWrap |
✅ Real-time path; |
localFallbackApplied |
maybeFinalizeStuckProgressCard sets true on MessageWrap |
✅ Consumed by isFallbackFinalized in Cell; not cleared by terminal update (harmless — idempotency guard only) |
decision.card |
computeState() → resolveEffectiveCardContent() → effective.card |
✅ Both VM and Cell use identical resolution; agreement confirmed |
isAgentProgressCard result |
cardLayout.ts:3-11 checks metadata.octo_layout === "agent_progress_v1" |
✅ Correct discriminator; not structural |
headerText |
getProgressCardHeaderText walks body→ColumnSet→columns[0]→items[0] |
✅ Returns null on structural anomaly; conservative |
fromUID |
MessageWrap.fromUID getter → message.fromUID (SDK field) |
✅ Always set for server-delivered messages |
streamNo |
message.streamNo (SDK field) |
✅ Canonical streaming detection; consistent with listener gate at :892 |
Blindspot Checklist
- C1 (Dual-path parity): N/A — no paired add/remove or subscribe/unsubscribe paths changed.
- C2 (Control-flow ordering/nested reuse): Clear —
maybeFinalizeStuckProgressCardis called once per new message arrival; no nested/double-invocation risk. - C3 (Authorization boundary): N/A — no auth/permission changes; fallback is purely client-side UI state.
- C4 (Auth lifecycle/container-member): N/A — no auth changes.
- C5 (Build/note ≠ runtime): Clear — CSS classes verified against actual CSS rules; one gap found (P2 above).
- C6 (Governance/docs self-consistency): N/A — no governance/docs changes.
Cross-round Blocker Check
N/A — first review round.
Things I checked that are fine
isAgentProgressCard(cardLayout.ts:3-11) correctly discriminates onmetadata.octo_layout === "agent_progress_v1", not on structural fields — robust against body shape variations.resolveEffectiveCardContent(resolveContent.ts:28-37) is a full-frame replacement (edit frame or original), not a merge — no risk of partial metadata leakage.elementText(agentProgressFallback.ts:37-51) handles bothTextBlockandRichTextBlockshapes; returns empty string for unknown types.- The
"fb:"prefix in the rendered key is correctly scoped to only affect the fallback transition, not unrelated card re-renders. MessageContentType.text(1, from SDK) andMessageContentTypeConst.interactiveCard(17, app-level) occupy the same number space without collision.- The
sendgetter (Model.tsx:358-360) correctly comparesfromUIDagainstWKApp.loginInfo.uid— self-sent text exclusion works. - Unit tests (
agentProgressFallback.test.ts) cover the pure-judgment functions with proper edge cases including malformed card structures and emoji boundaries. - DOM test (
agentProgressFallback.dom.test.ts) verifies the glyph replacement and class application on a real DOM tree.
Verdict: APPROVED
Well-structured, defensively coded client fallback that correctly scopes itself to local UI state without writing back to message data. The three P2 items are non-blocking: a dead CSS class (cosmetic gap), a known-scope limitation around reconnect, and single-card-per-sender fallback scope. None of these produce user-visible incorrect behavior beyond the stated design intent, and the server-side fix remains the authoritative terminal-state path.
[Octo-Q] verdict: APPROVE — No P0/P1 issues found. Three P2 observations are non-blocking design-scope items. Code is well-guarded, conservative in fallback triggering, and correctly isolated to local UI state.
…terminal frame The local fallback annotation set on a stuck progress card was never cleared when the real terminal frame later arrived, so the "no terminal update received" banner lingered over an already-rendered ✅/⚠️ card. When updateMessageByMessageExtras refreshes a fallback-marked card and its effective frame is terminal (terminal header, or no longer an agent_progress card as with the combined terminal+answer frame), clear localFallbackApplied so the real terminal state renders. Read receipts and non-terminal re-edits keep the flag, so read-receipt syncs never drop the banner on a still-stuck card. Co-authored-by: multica-agent <github@multica.ai>
|
Addressed the fallback race (2434a3e).
Scoped deliberately to terminal frames: read-receipt extra syncs (no Added |
What & why
Client-side fallback (方案 C) for stuck agent progress cards. When an assistant framework (openclaw / cc / hermes / codex) forgets to send the terminal frame for its type=17
agent_progress_v1card, the card stays stuck on "🤖 正在处理…" even though the assistant already delivered its type=1 final text. This makes octo-web close those zombie cards on its own.Root cause fix lives on the sender side (WS-97 Sub-1, openclaw-channel-octo); this is the defensive client backstop.
Behavior
On receiving a type=1 final text (non-stream, non-empty, not self):
senderId;Design notes
localFallbackAppliedflag onMessageWrap; the card tree and server content are untouched, so the sender-side fix (Sub-1) never conflicts — a real terminal frame just renders normally.console.debugfor troubleshooting.Tests
agentProgressFallback.test.ts— header extraction + terminal/non-terminal/idle logic (11 cases).agentProgressFallback.dom.test.ts— jsdom DOM overlay (⏳→Scope
Only octo-web. No IM server changes, no message-type changes. iOS / Android (Sub-3 / Sub-4) will follow this behavior once stable.
Closes #1034