Skip to content

Handle Feishu streaming media as visible results#517

Open
zhulijin1991 wants to merge 1 commit into
larksuite:mainfrom
zhulijin1991:codex/lark-media-visible-result
Open

Handle Feishu streaming media as visible results#517
zhulijin1991 wants to merge 1 commit into
larksuite:mainfrom
zhulijin1991:codex/lark-media-visible-result

Conversation

@zhulijin1991
Copy link
Copy Markdown
Contributor

Summary

  • Continue media delivery after routing streaming-card text payloads into the active card.
  • Mark successful media delivery as a visible streaming-card result.
  • Avoid the empty-reply fallback when a streaming turn successfully delivered media but had no text.

Root cause

In streaming-card mode, mixed text/media payloads returned immediately after the text went to the controller, so the media branch never ran. Media-only payloads did send through the static media path, but the controller had no visible-result signal, so the final card could still behave like the agent produced no displayable output.

Validation

  • pnpm test -- tests/reply-dispatcher-media.test.ts
  • pnpm exec prettier --check src/card/reply-dispatcher.ts src/card/reply-dispatcher-types.ts src/card/streaming-card-controller.ts tests/reply-dispatcher-media.test.ts
  • pnpm run typecheck
  • git diff --check

Copy link
Copy Markdown
Collaborator

@evandance evandance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core fix — removing the early return so media continues to deliver in streaming mode — is exactly right and addresses the bug reported in #515/#337.

One scope request before merge, applying a narrower version of the line I'm taking on #499: for fallback-text changes whose user impact we can't ground in real evidence, I'd rather not move to a new string without something concrete to point to. Specifically:

Please drop the new MEDIA_DELIVERY_FALLBACK_TEXT constant and the displayText ternary in streaming-card-controller.ts that selects between 'Media delivered.' and the existing EMPTY_REPLY_FALLBACK_TEXT. Both strings are reasonable card text for the rare media-only-finalization state; without user feedback I'd rather leave the existing fallback in place.

To be clear on what should stay: markMediaDelivered() and the visibleMediaDelivered flag are useful and should remain. Their other two responsibilities — capturing toolUseElapsed so media-only flows have correct footer timing, and suppressing the otherwise-misleading 'reply completed without visible text' warn log for successful media-only completions — are legitimate consequences of the bug fix that the existing finalization path otherwise can't handle correctly. The only change to the controller is the displayText ternary collapsing back to EMPTY_REPLY_FALLBACK_TEXT; the warn-suppression condition (!this.visibleMediaDelivered) stays.

That leaves the PR as: remove the early return, add textHandledByController flag to gate static text, keep markMediaDelivered() with the warn-suppression + timing-capture responsibilities, drop the new fallback constant and the displayText branch.

@evandance evandance added the changes requested Need do changes label May 16, 2026
@zhulijin1991 zhulijin1991 force-pushed the codex/lark-media-visible-result branch from 69f4f4c to d97a580 Compare May 17, 2026 07:59
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Updated in d97a580.\n\nChanges:\n- Dropped the new MEDIA_DELIVERY_FALLBACK_TEXT constant.\n- Collapsed the terminal displayText fallback back to EMPTY_REPLY_FALLBACK_TEXT.\n- Kept markMediaDelivered(), visibleMediaDelivered, tool-use elapsed capture, and the media-only warn suppression as requested.\n- Rebased onto current upstream/main.\n\nValidation:\n- pnpm test tests/reply-dispatcher-media.test.ts\n- pnpm typecheck\n\nReady for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Need do changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants