feat(im): enrich messages with reactions + output update_time#1095
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds EnrichReactions to batch-fetch and attach message reactions, conditionally includes formatted update_time in formatted messages, and adds a ChangesMessage Reactions and Formatting Enrichment
Sequence DiagramsequenceDiagram
participant User
participant Shortcut
participant EnrichReactions
participant IM_API
participant MessageMap
User->>Shortcut: run command (with or without --no-reactions)
Shortcut->>EnrichReactions: call (unless --no-reactions)
EnrichReactions->>MessageMap: traverse messages + thread_replies collect IDs
EnrichReactions->>IM_API: POST /open-apis/im/v1/messages/reactions/batch_query (batches of <=20)
IM_API-->>EnrichReactions: success_msg_reaction_counts + success_msg_reaction_details
EnrichReactions->>MessageMap: attach reactions.counts/details to matching messages
Shortcut-->>User: render messages (with reactions when present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/im/convert_lib/reactions_test.go (1)
16-83: ⚡ Quick winAdd a regression test for duplicate
message_idinputs.Please add one case where two entries share the same
message_idand assert both entries get identicalreactions. That will guard the in-place merge contract.Also applies to: 150-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/convert_lib/reactions_test.go` around lines 16 - 83, Extend TestEnrichReactions_Success to include a third message entry with the same message_id as the first (e.g., messages := []map[string]interface{}{{"message_id":"om_a"}, {"message_id":"om_b"}, {"message_id":"om_a"}}) then call EnrichReactions(runtime, messages) and assert that the third entry receives a reactions field identical to the first entry (compare the reactions map or its JSON serialization) to guard the in-place merge contract; apply the same duplicate-id case and assertion to the related test covering lines ~150-179 as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/convert_lib/reactions.go`:
- Around line 44-56: The code currently maps a single message map to each
message_id using idIndex (map[string]map[string]interface{}), so duplicate
message_id entries lose all but the first message; change idIndex to
map[string][]map[string]interface{} (or map[string][]int if you prefer storing
indexes) and in the initial loop (where idIndex and ids are populated) append
each msg to the slice for that id instead of overwriting/ignoring; then when you
attach reactions (the later blocks around where reactions are applied — the code
that iterates ids and sets msg["reactions"]) iterate over the slice for each id
and add the reactions to every message entry, ensuring duplicates all get the
reactions (apply the same change to the other similar blocks noted at the
review: the sections around lines 73-74 and 93-111).
---
Nitpick comments:
In `@shortcuts/im/convert_lib/reactions_test.go`:
- Around line 16-83: Extend TestEnrichReactions_Success to include a third
message entry with the same message_id as the first (e.g., messages :=
[]map[string]interface{}{{"message_id":"om_a"}, {"message_id":"om_b"},
{"message_id":"om_a"}}) then call EnrichReactions(runtime, messages) and assert
that the third entry receives a reactions field identical to the first entry
(compare the reactions map or its JSON serialization) to guard the in-place
merge contract; apply the same duplicate-id case and assertion to the related
test covering lines ~150-179 as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1683934c-2434-4040-a85b-97ab6cda0684
📒 Files selected for processing (8)
shortcuts/im/convert_lib/content_convert.goshortcuts/im/convert_lib/content_media_misc_test.goshortcuts/im/convert_lib/reactions.goshortcuts/im/convert_lib/reactions_test.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_messages_mget.goshortcuts/im/im_messages_search.goshortcuts/im/im_threads_messages_list.go
79e7474 to
b6970a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/convert_lib/content_convert.go`:
- Around line 162-163: The check for update_time should use an explicit string
assertion and trim rather than relying on interface{} != "" (which won't panic);
in the block where you read m["update_time"] (variable v) and set
msg["update_time"], change to assert vStr, ok := v.(string); if ok { vStr =
strings.TrimSpace(vStr); if vStr != "" { msg["update_time"] =
common.FormatTime(vStr) } } so FormatTime receives a clear string and empty
values are handled explicitly.
In `@shortcuts/im/convert_lib/reactions_test.go`:
- Around line 20-305: Tests in reactions_test.go use newBotConvertlibRuntime;
replace that with the repo-standard cmdutil.TestFactory(t, config) test factory
and ensure you isolate config by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) in each test (or the shared test helper) before creating the
factory; update each TestEnrichReactions_* (e.g. TestEnrichReactions_Success,
TestEnrichReactions_BatchSize, TestEnrichReactions_APIFailure, etc.) to call
t.Setenv(...) and to construct the runtime via cmdutil.TestFactory(t, config)
(or adapt newBotConvertlibRuntime to return a cmdutil.TestFactory-backed
runtime) so the tests follow the repo convention and use isolated config state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 478e9ad2-3187-401f-8bdd-2e264cb840ab
📒 Files selected for processing (14)
shortcuts/im/convert_lib/content_convert.goshortcuts/im/convert_lib/content_media_misc_test.goshortcuts/im/convert_lib/reactions.goshortcuts/im/convert_lib/reactions_test.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_messages_mget.goshortcuts/im/im_messages_search.goshortcuts/im/im_threads_messages_list.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-messages-list.mdskills/lark-im/references/lark-im-messages-mget.mdskills/lark-im/references/lark-im-messages-search.mdskills/lark-im/references/lark-im-reactions.mdskills/lark-im/references/lark-im-threads-messages-list.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-im/references/lark-im-reactions.md
- skills/lark-im/references/lark-im-threads-messages-list.md
- skills/lark-im/references/lark-im-messages-mget.md
b6970a1 to
2aa7c83
Compare
- Pull messages now auto-call im.reactions.batch_query and attach a reactions block (counts + details) to each message. Stops AI from misjudging "user already reacted" as "no response yet" and re-sending duplicate reactions. Server caps queries[] at 20 per call, so messages are split into batches of size <= 20. - Edited messages additionally surface update_time. The server echoes update_time == create_time for unedited messages too, so the field is only emitted when updated == true; otherwise every message output would look "edited". The value is read via an explicit string assertion + TrimSpace so empty strings are filtered properly (the previous `v != ""` was a no-op for non-string types). - All four message-pulling shortcuts (+messages-mget, +chat-messages-list, +messages-search, +threads-messages-list) get a --no-reactions opt-out flag for callers that want to skip the extra round-trip. - Each shortcut declares im:message.reactions:read on its UserScopes/BotScopes (or Scopes for the user-only search command) so the auth flow covers the new dependency. - Each shortcut's --dry-run output now lists the reactions/batch_query call (or omits it when --no-reactions is set), so callers can audit the full set of API calls before execution. - Warnings go through runtime.IO().ErrOut (forbidigo lint requires IOStreams over os.Stderr in shortcut code). - Duplicate message_id inputs (e.g. mget --message-ids om_a,om_a) attach the reactions block to every entry while still querying the API only once per distinct id. - EnrichReactions walks msg["thread_replies"] recursively, and mget/ chat-messages-list call it after ExpandThreadReplies, so replies receive reactions in the same batched call as their parent message. - When the batch_query call fails or returns per-message failures, the affected messages get reactions_error=true (mirroring the thread_replies_error flag from thread.go) so consumers can distinguish "fetch failed" from "no reactions exist" by reading stdout alone, without depending on the stderr warning channel. - lark-im skill docs: the default-enrichment contract lives in a standalone references/lark-im-message-enrichment.md so the generated SKILL.md can't strand it on regeneration. The four read references and the raw reactions API reference link to it, and the template source skill-template/domains/im.md carries a durable pointer. Change-Id: Ia9ea74b11945644262bb25c6503fb9b2003c6c98
2aa7c83 to
c1d3b66
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c1d3b66c37f50d2eaa64cb8384135e2ca03f3602🧩 Skill updatenpx skills add sammi-bytedance/larksuite-cli#feat/im-reactions-enrich-update-time -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
+ Coverage 67.91% 68.13% +0.21%
==========================================
Files 592 614 +22
Lines 55410 56525 +1115
==========================================
+ Hits 37631 38512 +881
- Misses 14669 14829 +160
- Partials 3110 3184 +74 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tches
Four related optimizations to the auto-enrichment side-fetches that run
on every +chat-messages-list / +messages-mget / +messages-search /
+threads-messages-list invocation. Each targets the same
latency-multiplier pattern: independent per-resource HTTP calls
dispatched in a strictly-serial loop, where each call has ~700ms-1s RTT
regardless of payload, so N resources turn into N × ~1s of stall before
the main message output can render. On busy chats the cumulative cost
pushed --no-reactions wall time above 14s — enough to trigger wall-clock
timeouts in wrapper agents even though the CLI itself never errored.
1) EnrichReactions: bounded-concurrency batches (cap = 4 in flight).
im.reactions.batch_query is server-capped at 20 queries[] per call,
so the id set is split into batches of <=20 and the batches now
dispatch under bounded concurrency. Stays at cap=4 because this
endpoint has an explicit gateway-layer rate limit (50/s + 1000/min)
that the cap stays well under. Each batch only writes to its own
message-id's map entries — concurrent batches never contend.
Stderr warnings serialize through a tiny mutex so partial lines from
concurrent goroutines don't interleave. Single-batch fast path
preserved.
2) ExpandThreadReplies: bounded-concurrency per-thread fetches (cap = 8
in flight) + batched sender resolution + thread-reply merge_forward
prefetch.
Phase 1 plans unique thread_ids in order (greedy budget allocation
against totalLimit), then concurrently fetches each thread's
replies. Phase 2 stays single-threaded: 2a) pre-fetches
merge_forward sub-messages for any thread reply that is itself a
merge_forward, 2b) FormatMessageItem per reply with that prefetch
attached, 2c) ONE batched ResolveSenderNames across every plan's
replies (was previously called per-thread, fanning out the contact
API request), 2d) AttachSenderNames + write thread_replies to each
plan's host outer message. Semantics preserved: first outer message
per thread_id remains the host; totalLimit honored (with a
documented mild pessimism); thread_replies_error: true on fetch
failure unchanged; thread_has_more: true propagated unchanged.
3) merge_forward sub-message expansion: shortcut-level concurrent
prefetch (cap = 8 in flight) + batched sub-item sender resolution
+ a fix for the error-masking bug.
The previous mergeForwardConverter.Convert issued one GET per
merge_forward message inline inside FormatMessageItem — so N
merge_forwards in a page took N × ~1.7s serial (measured: 5
merge_forwards × 1.7s = 8.5s of stall in a real chat, 67% of total
--no-reactions wall time).
New PrefetchMergeForwardSubItems pre-scans rawItems, picks out
merge_forward message_ids, and concurrently fetches each one's flat
sub-tree. The result map is threaded into FormatMessageItem via the
new FormatMessageItemWithMergePrefetch variant and ConvertContext's
new MergeForwardSubItems field. Convert first checks the prefetch
cache (fast path), falling back to its pre-existing inline GET when
no cache is present so non-shortcut callers (event subscribers,
ad-hoc tests, etc.) keep working unchanged.
Crucially, PrefetchMergeForwardSubItems also takes a nameCache and
runs ONE batched ResolveSenderNames across every sub-item it
fetched before returning. Without this step, each per-merge_forward
render in the FormatMessageItem loop would issue its own contact
API request for any uncached sender — re-introducing an N × ~400ms
serial stall (measured at 5 × ~400ms = ~2s extra in the
FormatMessageItem loop). Pre-populating the cache makes those
per-render ResolveSenderNames calls effective no-ops; the loop
dropped from ~2.3s to ~2ms on the same test chat.
Also switched fetchMergeForwardSubMessages from runtime.DoAPI to
runtime.DoAPIJSON. The old path checked only the presence of "data"
in the unmarshalled response, so a server response like
{"code": 2200, "msg": "Internal Error", ...} was reported as a
generic "empty data" string, hiding the real failure. DoAPIJSON
inspects the envelope's code and surfaces the real msg (and log_id
when present) as an ErrAPI, so merge_forward fetch failures now
produce content like "[Merged forward: fetch failed: API error
2200: Internal Error]" instead of the cryptic "empty data".
4) Empirical confirmation that mget can NOT batch-expand merge_forward:
passing 5 merge_forward ids to GET /open-apis/im/v1/messages/mget
returns exactly 5 items (msg_type=merge_forward each, no children
attached). The flat sub-tree only comes back from the per-id
GET /messages/{merge_forward_id} endpoint, so concurrent per-id
GETs (above) is the best the Feishu API surface allows.
Concurrency caps rationale:
- im.reactions.batch_query: 4 (explicit gateway 50/s + 1000/min
ceiling — stay conservative).
- GET /messages (thread expansion) and GET /messages/{id}
(merge_forward): 8 (no published per-app rate-limit at these
levels).
Tests:
- Reactions: TestEnrichReactions_BatchSize made order-tolerant; new
TestEnrichReactions_MultiBatchCorrectness (65 msgs → 4 batches
under bounded fan-out).
- Threads: pre-existing tests pass unchanged; new
TestExpandThreadRepliesMultiThreadConcurrent (5 distinct thread
roots, tagged-by-thread_id anti-contamination check).
- merge_forward: replaced single "empty data" test with two targeted
subtests — "empty_data_treated_as_no_children" verifies the new
"code:0 with no data field → empty slice + nil err" behavior;
"non-zero_code_surfaces_real_error" is a regression test for the
code:2200 production bug. New TestPrefetchMergeForwardSubItems
(mixed input of 5 merge_forwards + 2 non-merge_forwards, path-
derived child tags defeat goroutine cross-contamination).
- `go test -race -count=1 ./shortcuts/im/convert_lib/` clean.
- `gofmt -l .` / `go vet ./...` clean.
Docs:
- skills/lark-im/references/lark-im-message-enrichment.md: notes the
bounded concurrency in the reactions bullet.
Live measurement (group chat, page-size 50, 39 outer messages, 5
merge_forwards, 4 thread roots, 14 nested thread replies, 15 messages
with reactions; median of 3 runs):
before after delta
reactions ON 15.6s 8.1s -7.5s (-48%)
reactions OFF (no enrich) 14.3s 6.6s -7.7s (-54%)
Verified output correctness: all 5 merge_forwards expanded, all 4
thread roots attached their replies, reactions populated where the
server returned data, only the expected "reactions_partial_failed"
stderr warning for the per-message backend error from the larksuite#1095
contract.
Stage breakdown of the 14.3s --no-reactions baseline (one-off
instrumentation, then reverted):
before after
GET /messages (initial pull) ~1.7s ~1.7s (server)
Prefetch merge_forward + sender resolve - ~2.5s (new)
FormatMessageItem loop ~8.5s ~2ms (was: 5 × serial merge_forward + per-render contact API)
outer ResolveSenderNames + Attach ~0.5s ~0.5s
ExpandThreadReplies ~4s ~2.0s (was: 4 × serial thread GET + per-thread contact API)
Scope:
- shortcuts/im/convert_lib/reactions.go, thread.go, merge.go,
content_convert.go + their tests; 4 shortcut Execute hooks
(chat-messages-list, mget, search, threads-messages-list) to call
PrefetchMergeForwardSubItems and route through
FormatMessageItemWithMergePrefetch; one docs update.
- No framework-level changes.
Out of scope (separate follow-ups when needed):
- Decoupling --no-reactions from the im:message.reactions:read scope
pre-flight (currently required even when the flag opts out) — UX
change, not perf.
- Running PrefetchMergeForwardSubItems and ExpandThreadReplies in
parallel (they're independent and currently serial, adding ~1.5s
of unnecessary sequencing). Tractable but adds complexity around
nameCache locking; defer until needed.
Change-Id: I193519510a352b9dffd2069eb53dddf5ee3dffab
…tches (#1146) Follow-up to #1095. The reactions auto-enrichment shipped, but on busy chats the strictly-serial per-resource fetches in EnrichReactions, ExpandThreadReplies, and merge_forward expansion stretched the command's wall time above 14s — enough that wrapper agents (30–60s wall-clock budgets) saw timeouts even though the CLI itself never errored. This PR parallelizes all three with the same bounded-concurrency pattern, batches the follow-up contact-API sender resolution so it doesn't fan back out into a serial stall, and fixes two correctness bugs that surfaced during review. Scoped to convert_lib/{reactions,thread,merge,content_convert}.go + tests + the 4 shortcut Execute hooks + the reference doc. Change-Id: I0206d10ad204382170bd42aec67f82578923736e
…ite#1095) - Pull messages now auto-call im.reactions.batch_query and attach a reactions block (counts + details) to each message. Stops AI from misjudging "user already reacted" as "no response yet" and re-sending duplicate reactions. Server caps queries[] at 20 per call, so messages are split into batches of size <= 20. - Edited messages additionally surface update_time. The server echoes update_time == create_time for unedited messages too, so the field is only emitted when updated == true; otherwise every message output would look "edited". The value is read via an explicit string assertion + TrimSpace so empty strings are filtered properly (the previous `v != ""` was a no-op for non-string types). - All four message-pulling shortcuts (+messages-mget, +chat-messages-list, +messages-search, +threads-messages-list) get a --no-reactions opt-out flag for callers that want to skip the extra round-trip. - Each shortcut declares im:message.reactions:read on its UserScopes/BotScopes (or Scopes for the user-only search command) so the auth flow covers the new dependency. - Each shortcut's --dry-run output now lists the reactions/batch_query call (or omits it when --no-reactions is set), so callers can audit the full set of API calls before execution. - Warnings go through runtime.IO().ErrOut (forbidigo lint requires IOStreams over os.Stderr in shortcut code). - Duplicate message_id inputs (e.g. mget --message-ids om_a,om_a) attach the reactions block to every entry while still querying the API only once per distinct id. - EnrichReactions walks msg["thread_replies"] recursively, and mget/ chat-messages-list call it after ExpandThreadReplies, so replies receive reactions in the same batched call as their parent message. - When the batch_query call fails or returns per-message failures, the affected messages get reactions_error=true (mirroring the thread_replies_error flag from thread.go) so consumers can distinguish "fetch failed" from "no reactions exist" by reading stdout alone, without depending on the stderr warning channel. - lark-im skill docs: the default-enrichment contract lives in a standalone references/lark-im-message-enrichment.md so the generated SKILL.md can't strand it on regeneration. The four read references and the raw reactions API reference link to it, and the template source skill-template/domains/im.md carries a durable pointer. Change-Id: Ia9ea74b11945644262bb25c6503fb9b2003c6c98
…tches (larksuite#1146) Follow-up to larksuite#1095. The reactions auto-enrichment shipped, but on busy chats the strictly-serial per-resource fetches in EnrichReactions, ExpandThreadReplies, and merge_forward expansion stretched the command's wall time above 14s — enough that wrapper agents (30–60s wall-clock budgets) saw timeouts even though the CLI itself never errored. This PR parallelizes all three with the same bounded-concurrency pattern, batches the follow-up contact-API sender resolution so it doesn't fan back out into a serial stall, and fixes two correctness bugs that surfaced during review. Scoped to convert_lib/{reactions,thread,merge,content_convert}.go + tests + the 4 shortcut Execute hooks + the reference doc. Change-Id: I0206d10ad204382170bd42aec67f82578923736e
Summary
im.reactions.batch_queryto attach a reactions block (counts + details) back onto each message. Fixes the duplicate-reaction problem where the AI misjudges "user already reacted → still thinks no response was given".update_time(the Lark API already returns this field; the CLI just never surfaced it).+chat-messages-list/+messages-search/+messages-mget/+threads-messages-list) gain a--no-reactionsopt-out flag.queries[]limit is 20, so requests are automatically split into batches when exceeded; any batch failure only emits a stderr warning and does not block the main output.Output shape
Only attached when the server returns data; the field is omitted for messages without reactions.
Test plan
convert_lib/reactions_test.go(success / batch=25→[20,5] / API failure / empty input / skip items missing message_id)convert_lib/content_media_misc_test.go(update_time present / absent)make unit-test/go vet ./.../gofmt -l ./go mod tidyall clean+chat-messages-listdefault enrich: retrieved messages with 4 reaction types, complete counts + details+chat-messages-list --no-reactions: reactions field absent, update_time still output+messages-search: 3 hits / 2 with reactions+messages-mget: single message with 4 counts + 4 details, update_time2026-05-11 14:51Rate limiting notes
im.reactions.batch_queryserver-side currently only has the OPL gateway layer fallback of 50/s + 1000/min; the larkim/openapi business layer has not yet integrated multi-dimensional rate limiting.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation