Skip to content

fix(im): cap basic_batch user_ids at 10 per API limit#551

Merged
YangJunzhou-01 merged 1 commit intolarksuite:mainfrom
haozhenghua-code:fix/basic-batch-size
Apr 18, 2026
Merged

fix(im): cap basic_batch user_ids at 10 per API limit#551
YangJunzhou-01 merged 1 commit intolarksuite:mainfrom
haozhenghua-code:fix/basic-batch-size

Conversation

@haozhenghua-code
Copy link
Copy Markdown
Contributor

@haozhenghua-code haozhenghua-code commented Apr 18, 2026

Summary

The POST /contact/v3/users/basic_batch endpoint accepts 1~10 user_ids per request (doc). batchResolveByBasicContact in shortcuts/im/convert_lib/helpers.go was chunking by 50, so whenever the user-identity sender-name resolver had >10 unresolved IDs, the single oversized request was rejected and the whole batch was silently skipped — leaving sender names empty in the output of im +chat-messages-list, +messages-search, +messages-mget, +threads-messages-list, etc.

Changes

  • shortcuts/im/convert_lib/helpers.go: lower batchSize from 50 to 10 in batchResolveByBasicContact and add a comment citing the API limit. The sibling batchResolveUsers (calls the different GET /contact/v3/users/batch endpoint, cap 50) is untouched.
  • shortcuts/im/convert_lib/helpers_test.go: add TestBatchResolveByBasicContactRespectsAPILimit that feeds 25 missing IDs and asserts the mock server sees three requests of size 10 / 10 / 5 and that every resolved name ends up in the name map.

Test Plan

Automated

  • go test ./shortcuts/im/convert_lib/... passes (new + existing tests)
  • go vet ./... and go build ./... clean

End-to-end (built binary, live Feishu API)

Exercised the real im +chat-messages-list flow on two internal group chats (chat IDs and user identifiers redacted). Compared the number of unique user senders vs. how many ended up with their display name populated after ResolveSenderNamesAttachSenderNames.

Case Identity batchSize Unique user senders Names resolved Names missing
Positive — user identity, after fix user 10 31 31 0
Positive — bot identity, after fix (unchanged batchResolveUsers path, different API) bot n/a 4 4 0
Negative — user identity, reverted batchSize to 50 (reproduces the bug) user 50 31 0 31

The negative run demonstrates the bug was real: with 31 unresolved IDs packed into a single oversized request, the server rejects it and the resolver's break leaves the whole name map empty. After the fix, the same 31 IDs are split across 4 batches (10/10/10/1) and every name comes back.

Related Issues

  • None

The POST /contact/v3/users/basic_batch endpoint caps user_ids at 1~10
per request, but batchResolveByBasicContact was chunking by 50. When
user identity needed to resolve >10 unresolved sender names, the
single oversized request was rejected, causing the batch resolver to
bail out and leave sender names empty for the rest.

Lower batchSize to 10 and add a unit test that exercises 25 missing
IDs and asserts they are sent as 10 / 10 / 5.
@github-actions github-actions bot added domain/im PR touches the im domain size/M Single-domain feat or fix with limited business impact labels Apr 18, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 18, 2026

📝 Walkthrough

Walkthrough

Reduced the batch size constant from 50 to 10 in batchResolveByBasicContact for the /open-apis/contact/v3/users/basic_batch endpoint, resulting in smaller per-request batches. Added a corresponding unit test to verify the batching pattern with 25 IDs produces expected batch splits.

Changes

Cohort / File(s) Summary
Batch Size Adjustment
shortcuts/im/convert_lib/helpers.go
Changed batchSize constant from 50 to 10 in batchResolveByBasicContact to align with per-request API limits.
Batching Verification Test
shortcuts/im/convert_lib/helpers_test.go
Added TestBatchResolveByBasicContactRespectsAPILimit to validate the new batch size of 10 IDs per request and verify batching pattern [10, 10, 5] for 25 IDs. Extended imports with encoding/json and io.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

domain/im, size/M

Suggested reviewers

  • sammi-bytedance
  • YangJunzhou-01
  • zgz2048

Poem

🐰 Ten by ten, we hop along,
Batching smaller, staying strong,
No more fifty in a heap,
Ten per request, a measured leap! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: capping the basic_batch user_ids limit at 10 to match the API's per-request constraint.
Description check ✅ Passed The pull request description comprehensively addresses all required template sections with detailed context, code changes, test plan with automated and manual verification results, and clear problem motivation.

✏️ 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

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@c1d6042). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #551   +/-   ##
=======================================
  Coverage        ?   59.96%           
=======================================
  Files           ?      388           
  Lines           ?    33147           
  Branches        ?        0           
=======================================
  Hits            ?    19878           
  Misses          ?    11400           
  Partials        ?     1869           

☔ 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
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5d200a6c6e3a1b800a3694729654471e61f34bba

🧩 Skill update

npx skills add haozhenghua-code/larksuite-cli#fix/basic-batch-size -y -g

@YangJunzhou-01 YangJunzhou-01 merged commit db7d3cb into larksuite:main Apr 18, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/im PR touches the im domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants