Skip to content

Clarify empty Feishu card fallback#499

Closed
zhulijin1991 wants to merge 3 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-empty-reply-fallback
Closed

Clarify empty Feishu card fallback#499
zhulijin1991 wants to merge 3 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-empty-reply-fallback

Conversation

@zhulijin1991
Copy link
Copy Markdown
Contributor

@zhulijin1991 zhulijin1991 commented May 11, 2026

Summary

  • Replace the terse empty-reply fallback text with a short neutral fallback.
  • Add controller-path coverage for the empty-reply final-card path.

Validation

  • pnpm vitest run tests/reply-dispatcher-empty-fallback.test.ts
  • pnpm typecheck

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 11, 2026

CLA assistant check
All committers have signed the CLA.

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.

Thanks — agreed 'Done.' is misleading when the run produces no visible text. The constant renders in the final streaming card body (streaming-card-controller.ts:693), so this surface is end-user-facing. Two follow-up thoughts, neither blocking.

On the wording: the replacement is more honest but on the heavy side for a chat-bubble fallback — ~50 chars vs 'Done.' at 5, English only (visible to users in any locale), and uses agent-internal vocabulary (run, displayable, final reply) end users don't share. Something terser and locale-neutral would communicate the same fact more cleanly: e.g. '(no reply)', or possibly '' paired with the existing log.warn('reply completed without visible text…') on line 695 if empty final card bodies are known to be safe.

On the test: expect(EMPTY_REPLY_FALLBACK_TEXT).toBe('<same literal>') / .not.toBe('Done.') only pin the literal value. A regression test that drives the controller down the empty-reply path (empty completedText + empty accumulatedText) and asserts the rendered card body contains the fallback would actually catch future refactors of onIdle() that skip the fallback path.

Both are quality improvements rather than fixes — fine either landing this as-is and tackling separately, or folding into this PR.

@evandance evandance added feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming labels May 12, 2026
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Thanks, that makes sense. I agree the current replacement is too verbose for an end-user card and leaks agent-internal wording.

I will fold this into the PR with a terser, locale-neutral fallback, likely (no reply), instead of keeping the current sentence. I will also replace the literal-only assertion with a controller-path regression that drives the empty-reply path (completedText + accumulatedText empty) and asserts the rendered card body receives the fallback.

@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Updated in 3e93a93.

I kept the terse (no reply) fallback and removed the literal-only assertion from the new test, so the coverage now stays focused on driving StreamingCardController.onIdle() through the empty completed/accumulated text path and asserting the rendered card contains the fallback.

Validation:

  • pnpm test tests/reply-dispatcher-empty-fallback.test.ts
  • pnpm typecheck

@evandance
Copy link
Copy Markdown
Collaborator

Thanks for working through the iterations on this one — and apologies for not flagging this scope question in the first round.

After a closer look, this fallback fires only in rare edge cases (NO_REPLY token paths, tool-only runs with no text), and changing the user-visible string here is a UX-copy judgment that we can't ground in real user evidence yet. For fallback-copy changes specifically, I'd rather take a conservative line — keeping the existing wording until there's a concrete user report or data pointing to a better one, rather than moving to a new string without that evidence.

I'd like to close this rather than push for further iterations. To be clear, this isn't a comment on the work itself or on bug-fix or feature contributions more broadly; it's specifically about unevidenced fallback-copy changes. Your other Feishu work has been landing exactly because those fixes have concrete user impact, and I'd love to keep seeing more of that. Please don't let this close deter the next PR.

@evandance evandance closed this May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants