perf(im): parallelize reactions, thread_replies, and merge_forward fetches

[sammi-bytedance]

Summary

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.

Changes

1. EnrichReactions — bounded-concurrency batches (cap = 4)

im.reactions.batch_query is server-capped at 20 queries[] per call. Batches now dispatch under bounded concurrency. Cap stays at 4 because this endpoint has an explicit gateway-layer rate limit (50/s + 1000/min).

Safety invariant documented inline: collectMessageNodes dedups ids on first-seen, so every batch sub-slice carries each id at most once. Different batches therefore operate on disjoint idIndex buckets → disjoint message-map pointers → no concurrent writes to the same map. Race detector verifies; TestEnrichReactions_DuplicateMessageID + TestEnrichReactions_MultiBatchCorrectness cover the round-trip.

2. ExpandThreadReplies — bounded-concurrency fetches (cap = 8) + batched sender resolution + correct totalLimit accounting

Two-phase: Phase 1 plans every unique thread_id and concurrently fetches all of them. Phase 2 is single-threaded: pre-fetch merge_forward sub-messages for thread replies, format, one batched ResolveSenderNames across every reply, attach.

totalLimit correctness (regression fix): an earlier draft deducted the planned per-thread ceiling (perThread, default 50) from totalLimit upfront, which silently dropped later threads in chats with many short threads. Concretely: 12 threads × 3 actual replies each = 36 total, but planned-ceiling accounting (12 × 50 = 600) would have exceeded the default totalLimit=500 after just 10 threads and silently skipped the last two. New code budgets against actual returned counts post-fetch: threads past a fully-drained budget have their slice cleared to an empty (non-nil) slice so the attach loop leaves the host alone; threads whose count straddles the budget boundary get truncated with thread_has_more flagged. Empty-slice vs nil-slice is the discriminator between "budget-skipped (success)" and "fetch error (mark thread_replies_error)". Regression covered by TestExpandThreadRepliesTotalLimitUsesActualCounts and TestExpandThreadRepliesTruncatesOnBudgetBoundary. Cost: under budget overflow, threads beyond the boundary are still fetched then discarded — bounded by perThread records each, an acceptable price for not losing data silently.

3. merge_forward — shortcut-level concurrent prefetch (cap = 8) + batched sub-item sender resolution + error-masking fix + correct prefetch-failure fallback

The previous Convert issued one GET per merge_forward inline inside FormatMessageItem — N merge_forwards × ~1.7s serial (5 merge_forwards = ~8.5s, 67% of total --no-reactions wall time).

PrefetchMergeForwardSubItems now pre-scans rawItems, picks out merge_forward message_ids, and concurrently fetches each sub-tree. Threaded into FormatMessageItem via the new FormatMessageItemWithMergePrefetch variant + ConvertContext.MergeForwardSubItems. Convert checks the prefetch cache first; missing key falls through to the inline GET so non-shortcut callers (event subscribers, ad-hoc tests) keep working.

Prefetch-failure handling (regression fix): an earlier draft inserted result[id] = nil on failure, which made Convert's if cached, ok := m[id]; ok { renderTree(cached) } path hit ok=true, cached=nil and emit an empty <forwarded_messages> tree — silently swallowing the user-visible [Merged forward: fetch failed: ...] string that the inline path used to produce. The new code does not insert the key at all on failure; ok=false triggers the inline-fetch slow path which (a) gets a second attempt at the GET, (b) if that also fails, produces the proper error string. Regression covered by TestPrefetchMergeForwardSubItemsFailureFallsThroughToInlineFetch.

Critical perf refinement: PrefetchMergeForwardSubItems also takes a nameCache and runs one batched ResolveSenderNames across every sub-item it fetched before returning. Without this, each per-merge_forward render in the loop would issue its own contact API request for any uncached sender — re-introducing an N × ~400ms serial stall (~2s extra in FormatMessageItem). Pre-populating the cache makes those per-render calls no-ops; the FormatMessageItem loop dropped from ~2.3s → ~2ms on the same test chat.

Also: switched fetchMergeForwardSubMessages from low-level runtime.DoAPI to runtime.DoAPIJSON. The old path only checked for the presence of data in the body, so a real server error like {"code": 2200, "msg": "Internal Error", ...} was reported as a generic "empty data" string. DoAPIJSON surfaces the real msg (and log_id) — [Merged forward: fetch failed: API error 2200: Internal Error] instead.

4. Confirmed: mget cannot batch-expand merge_forward

Empirically verified — GET /open-apis/im/v1/messages/mget with 5 merge_forward IDs returns exactly 5 items (all msg_type=merge_forward, 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 is the best the Feishu API surface allows.

Concurrency caps

endpoint	cap	reasoning
im.reactions.batch_query	4	explicit gateway 50/s + 1000/min ceiling
GET /messages (thread expansion)	8	no published per-app rate-limit at these levels
GET /messages/{id} (merge_forward)	8	no published per-app rate-limit at these levels

Test Plan

• Reactions: TestEnrichReactions_BatchSize made order-tolerant; new TestEnrichReactions_MultiBatchCorrectness (65 msgs → 4 batches, every msg correctly attributed).
• Threads: pre-existing tests pass; new TestExpandThreadRepliesMultiThreadConcurrent (5 distinct thread roots, tagged-by-thread_id anti-contamination); new TestExpandThreadRepliesTotalLimitUsesActualCounts (12 short threads × 3 replies vs totalLimit=100 — all 12 must attach); new TestExpandThreadRepliesTruncatesOnBudgetBoundary (3 threads × 4 replies vs totalLimit=10 — third thread truncated to 2 + thread_has_more=true).
• merge_forward: replaced single "empty data" test with empty_data_treated_as_no_children + non-zero_code_surfaces_real_error (regression for the code:2200 production bug); new TestPrefetchMergeForwardSubItems (mixed 5 merge_forwards + 2 non-merge_forwards, path-derived child tags defeat goroutine cross-contamination); new TestPrefetchMergeForwardSubItemsFailureFallsThroughToInlineFetch (regression for the silent-empty-tree bug — confirms failed ids are absent from the prefetch map AND Convert produces the proper error string via inline retry).
• go test -race -count=1 ./shortcuts/im/convert_lib/ clean.
• gofmt -l . / go vet ./... clean.
• Live test on a real group chat (page-size 50, 39 outer messages, 5 merge_forwards, 4 thread roots with 14 nested replies, 15 messages with reactions). Output correctness verified — all merge_forwards expanded, all thread roots resolved with replies attached, reactions populated where the server returned data, only the expected reactions_partial_failed warning on stderr.

Wall-time, median of 3 runs

variant	before this PR	this PR	delta
reactions ON	15.6s	8.1s	−7.5s (−48%)
--no-reactions	14.3s	6.6s	−7.7s (−54%)

Stage breakdown (one-off instrumentation, then reverted)

stage	14.3s baseline	6.6s after
GET /messages	~1.7s	~1.7s
PrefetchMergeForwardSubItems (5 GETs + batched contact API)	—	~2.5s
FormatMessageItem loop	~8.5s (5 × serial merge_forward + per-render contact)	~2ms
outer ResolveSenderNames + Attach	~0.5s	~0.5s
ExpandThreadReplies (4 thread GETs + per-thread contact)	~4s	~2.0s

Related Issues

• Follow-up to feat(im): enrich messages with reactions + output update_time #1095 (initial reactions enrichment).
• 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 sequential, adding ~1.5s of avoidable sequencing). Adds nameCache-locking complexity; defer until needed.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Deduplicate IDs and batch fetch reactions with a single-batch fast path or bounded-concurrency multi-batch fan-out (serialized warnings). ExpandThreadReplies now plans per-thread limits, fetches replies concurrently with a cap, formats sequentially, resolves names in one batch, and attaches results. Tests and docs updated.

Changes

Reactions batching and concurrency

Layer / File(s)	Summary
Imports and concurrency constant shortcuts/im/convert_lib/reactions.go	Add io and sync imports and declare reactionsBatchQueryConcurrency to cap in-flight batch_query requests.
EnrichReactions batching and concurrent dispatch shortcuts/im/convert_lib/reactions.go	Deduplicate message IDs into batches (≤ reactionsBatchQueryMaxQueries). Single-batch fast path preserves serial behavior; multi-batch path fans out fetches with bounded concurrency via a semaphore and shares a stderr mutex to avoid interleaved warnings.
fetchReactionsBatch signature and warnReactions helper shortcuts/im/convert_lib/reactions.go	Add optional stderrMu *sync.Mutex parameter to fetchReactionsBatch. Replace direct stderr writes with warnReactions(stderrMu, ...) which conditionally locks the mutex to serialize warning output.
Concurrent batching tests shortcuts/im/convert_lib/reactions_test.go	Update TestEnrichReactions_BatchSize to collect observed batch sizes under a mutex and sort before asserting; add TestEnrichReactions_MultiBatchCorrectness to exercise the multi-batch path and verify per-message reactions attachment and expected batch count.
Enrichment contract documentation skills/lark-im/references/lark-im-message-enrichment.md	Document id-set batching (≤20 ids per batch) and bounded concurrency (≤4 in flight) for im.reactions.batch_query; maintain that reactions is attached only when the server returns reaction data and thread_replies follow same batching semantics.

Thread replies planning and concurrent fetch

Layer / File(s)	Summary
Thread imports and concurrency constant shortcuts/im/convert_lib/thread.go	Add sync import and declare threadRepliesFetchConcurrency; update function comment to describe two-phase planning/fetching and budget semantics.
Plan thread reply allocation shortcuts/im/convert_lib/thread.go	Enumerate unique thread_ids in first-seen order and greedily allocate per-thread reply limits from totalLimit until exhausted, recording host message per plan.
Concurrent fetch phase and result collection shortcuts/im/convert_lib/thread.go	Introduce plan/result structs, use single-plan fast path or spawn semaphore-bounded goroutines to fetch each planned thread concurrently and store per-plan results.
Format replies and batched name resolution shortcuts/im/convert_lib/thread.go	Format fetched replies sequentially (FormatMessageItem) and perform a single batched ResolveSenderNames across all replies to populate nameCache.
Attach replies and set flags shortcuts/im/convert_lib/thread.go	Attach resolved replies back to each host message, call AttachSenderNames per plan, and set thread_replies_error / thread_has_more according to fetch outcomes.
Concurrency-focused thread tests shortcuts/im/convert_lib/thread_test.go	Add tests that count GET calls per distinct thread under mutex protection and assert one fetch per thread and correct reply attachment to originating messages.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• larksuite/cli#1095: Modifies reactions enrichment pipeline—EnrichReactions batching behavior and reactions attachment/handling per message_id, including stderr warning serialization.

Suggested labels

size/L

Suggested reviewers

• YangJunzhou-01

Poem

🐰 I hop through batches, small and neat,
Four fetchers racing to avoid defeat.
When warnings flutter, I tuck them tight,
A mutex hug keeps lines polite.
Tests cheer the run — reactions complete!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title 'perf(im): parallelize reactions, thread_replies, and merge_forward fetches' accurately summarizes the main performance improvements across three concurrent components in the changeset.
Description check	✅ Passed	PR description is comprehensive and well-structured, covering all required template sections: clear summary of changes, detailed list of implementation changes across 4 components, thorough test plan with checkmarks, and related issues linked.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/common/runner.go`:
- Around line 818-825: The checkScopePrereqs call can still panic because
checkScopePrereqs assumes f.Credential is non-nil; update the pre-check in
checkShortcutScopes (the block that currently returns when f==nil ||
config==nil) to also return nil when f.Credential == nil so scope prechecks are
skipped for unwired credential resolvers, or alternatively add the same
nil-guard at the start of checkScopePrereqs to avoid dereferencing f.Credential
in checkScopePrereqs (which calls f.Credential.ResolveToken).

In `@shortcuts/im/im_chat_messages_list.go`:
- Around line 101-113: The current helper buildChatMessageListRequest silently
normalizes invalid --page-size values (e.g. non-integers or out-of-range
numbers); change buildChatMessageListRequest so it explicitly parses
runtime.String("page-size") with strconv.Atoi and returns a clear error if
parsing fails or the value is outside the allowed min/max instead of clamping
it, and keep Validate (which calls buildChatMessageListRequest) returning that
error so callers are rejected rather than run with a different page size.

🪄 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: fb5d326f-722f-41b1-b19b-e3340c4086b7

📥 Commits

Reviewing files that changed from the base of the PR and between bbef3cb and 3bf3526.

📒 Files selected for processing (8)

• shortcuts/common/runner.go
• shortcuts/im/convert_lib/reactions.go
• shortcuts/im/convert_lib/reactions_test.go
• shortcuts/im/im_chat_messages_list.go
• shortcuts/im/im_messages_mget.go
• shortcuts/im/im_messages_search.go
• shortcuts/im/im_threads_messages_list.go
• skills/lark-im/references/lark-im-message-enrichment.md

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

skills/lark-im/references/lark-im-message-enrichment.md (1)

13-13: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Scope requirement text is now stale versus --no-reactions behavior.

This line still says the reactions scope is pre-flight enforced via unconditional UserScopes/BotScopes, but this PR’s objective is conditional enforcement (scope check only when --no-reactions is false). Please update this section to avoid contradicting runtime behavior.

Suggested doc fix

-The default enrichment requires `im:message.reactions:read`, already declared in each shortcut's `UserScopes` / `BotScopes` (or `Scopes` for the user-only search command), so the framework's pre-flight check surfaces a `missing_scope` error before the request is sent.
+When reactions enrichment is enabled (default), shortcuts require `im:message.reactions:read` and the pre-flight check can surface `missing_scope` before sending the reactions request. With `--no-reactions`, this scope is not required.

🤖 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 `@skills/lark-im/references/lark-im-message-enrichment.md` at line 13, The
documentation incorrectly states that the reactions scope
(im:message.reactions:read) is always pre-flight enforced via
UserScopes/BotScopes; update the text in Lark IM message enrichment to reflect
that scope enforcement is conditional based on the CLI flag --no-reactions —
i.e., explain that the framework will check for im:message.reactions:read only
when --no-reactions is false and that pre-flight missing_scope errors will only
occur in that case, while also keeping a note that bots registered before the
scope was added still require incremental authorization in the Feishu developer
console and users can run the shown command for authorization.

🤖 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.

Outside diff comments:
In `@skills/lark-im/references/lark-im-message-enrichment.md`:
- Line 13: The documentation incorrectly states that the reactions scope
(im:message.reactions:read) is always pre-flight enforced via
UserScopes/BotScopes; update the text in Lark IM message enrichment to reflect
that scope enforcement is conditional based on the CLI flag --no-reactions —
i.e., explain that the framework will check for im:message.reactions:read only
when --no-reactions is false and that pre-flight missing_scope errors will only
occur in that case, while also keeping a note that bots registered before the
scope was added still require incremental authorization in the Feishu developer
console and users can run the shown command for authorization.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce954f56-8d6e-4005-aecd-b2b0e6db99d0

📥 Commits

Reviewing files that changed from the base of the PR and between 1455dd8 and 3e1e371.

📒 Files selected for processing (3)

• shortcuts/im/convert_lib/reactions.go
• shortcuts/im/convert_lib/reactions_test.go
• skills/lark-im/references/lark-im-message-enrichment.md

🚧 Files skipped from review as they are similar to previous changes (2)

• shortcuts/im/convert_lib/reactions.go
• shortcuts/im/convert_lib/reactions_test.go

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@skills/lark-im/references/lark-im-message-enrichment.md`:
- Line 7: The examples mention batching for reactions fetched via
im.reactions.batch_query and thread_replies using a 20-ID server cap but the
math is off: for “page 50 + ~500 expanded thread replies” (~550 IDs) the count
should be 28 batches (550 / 20 = 27.5 → 28), not ~25; update the example text to
say "28 batches" (or adjust the example numbers to match 25 batches) so the
described batching for reactions and thread_replies aligns with the 20-ID cap
and bounded concurrency behavior.

🪄 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: d30576e6-7e01-4241-9fc8-54bcdd74ab47

📥 Commits

Reviewing files that changed from the base of the PR and between 3e1e371 and fc9b28c.

📒 Files selected for processing (3)

• shortcuts/im/convert_lib/reactions.go
• shortcuts/im/convert_lib/reactions_test.go
• skills/lark-im/references/lark-im-message-enrichment.md

🚧 Files skipped from review as they are similar to previous changes (1)

• shortcuts/im/convert_lib/reactions_test.go

[codecov]

Codecov Report

❌ Patch coverage is 80.33708% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.66%. Comparing base (b91f6a2) to head (72a8421).

Files with missing lines	Patch %	Lines
shortcuts/im/convert_lib/merge.go	70.14%	16 Missing and 4 partials ⚠️
shortcuts/im/convert_lib/thread.go	91.30%	3 Missing and 3 partials ⚠️
shortcuts/im/convert_lib/reactions.go	87.50%	2 Missing and 1 partial ⚠️
shortcuts/im/im_chat_messages_list.go	0.00%	2 Missing ⚠️
shortcuts/im/im_messages_mget.go	0.00%	2 Missing ⚠️
shortcuts/im/im_threads_messages_list.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
+ Coverage   68.63%   68.66%   +0.03%     
==========================================
  Files         625      625              
  Lines       58386    58514     +128     
==========================================
+ Hits        40071    40177     +106     
- Misses      15027    15046      +19     
- Partials     3288     3291       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@72a8421933135553961977361996cf54518ff257

🧩 Skill update

npx skills add sammi-bytedance/larksuite-cli#perf/im-reactions-parallel-batches -y -g