feat: resolve bot sender names in message list#823
Conversation
Implement bot/app sender name enrichment for IM message listing. - Extend ResolveSenderNames to collect sender_type=app|bot IDs in addition to users - Fetch app/bot display names via application v6 get endpoint and merge into the shared cache - Prefer bot identity when available (falls back to current identity) and keep failures non-fatal - Update unit tests to cover bot/app sender name resolution Co-Authored-By: Aime <aime@bytedance.com> Change-Id: I2cfc68abc4af90e9377cd2ad3c59f05cb7cfd12d
github-actions
Bot
added
domain/im
📝 WalkthroughWalkthrough
ChangesApp/Bot Sender Name Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
zhangzhewei seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
shortcuts/im/convert_lib/helpers.go (4)
75-85: ⚡ Quick winRefresh the
ResolveSenderNamesdoc block to mention bot/app resolution.The doc currently describes only Step 1 (mentions) and Step 2 (contact batch API for users), but the body now also has a Step 3 that resolves app/bot senders via the application API. Worth bringing the comment in line with the new behavior — including the API permissions required (e.g.,
application:application:readonly) so callers aren't surprised when bot/app names silently fail to resolve.📝 Suggested doc tweak
// ResolveSenderNames batch-resolves sender open_ids to display names. // The cache map is used to share already-resolved IDs across calls; newly resolved // names are written back into it. Pass an empty map if no prior cache exists. // // Step 1: extract names from message mentions (free, no API call). -// Step 2: for remaining unresolved IDs, call contact batch API (requires contact:user.base:readonly). +// Step 2: for remaining user sender IDs, call contact batch API +// (requires contact:user.base:readonly). +// Step 3: for remaining bot/app sender IDs, call application API +// (requires application:application:readonly; routed via bot identity when available). // Silently returns partial results on API error.🤖 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/helpers.go` around lines 75 - 85, Update the ResolveSenderNames doc block to reflect the current three-step behavior: Step 1 extracts display names from message mentions, Step 2 batch-resolves user IDs via the contact API (requires contact:user.base:readonly), and Step 3 resolves bot/app senders via the application API (requires application:application:readonly); also note that newly resolved names are written back into the provided cache map, and that the function may return partial results if any API call fails.
247-251: 💤 Low valueConfirm whether the
data.app_nametop-level fallback is reachable.The application v6 GET endpoint nests the app metadata under
data.app, sodata["app_name"]at line 250 looks like dead defensive code. If you've actually observed a response shape whereapp_namelives at the top level (e.g., different API version or routing), consider adding a comment noting that; otherwise the fallback can go.🤖 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/helpers.go` around lines 247 - 251, The fallback to data["app_name"] appears unreachable given the v6 API nests app metadata under data["app"]; verify by checking observed response shapes and, if no responses place app_name at the top level, remove the fallback lines (the cast of data["app_name"] and the alternate assignment) and rely on app["app_name"]; otherwise keep the fallback but add a clarifying comment above this block stating which API versions or endpoints produce a top-level app_name so future readers know it's intentional (referencing the variables data, app and the app_name assignment in helpers.go).
240-256: 💤 Low valueFunction name
batchResolveAppsis misleading — it's per-ID serial.There's no batch endpoint here (one HTTP request per app id, sequentially). For a chat-messages-list with many distinct bot senders, this becomes N round-trips on the critical path. The chill-mode acceptance is fine, but worth either renaming (e.g.,
resolveAppsSerially) for honesty, or adding a small concurrency layer (errgroupwith a low limit) if you expect more than a handful of bot senders per page.🤖 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/helpers.go` around lines 240 - 256, The function batchResolveApps is misleading because it performs serial per-ID requests; either rename it to resolveAppsSerially (and update all callers) or add limited concurrency: use an errgroup (or goroutine pool) to issue doAPIJSONAsBotIfPossible calls concurrently for elements of appIDs while protecting writes to nameMap with a mutex and bounding parallelism (e.g., a semaphore channel or worker pool) to avoid overwhelming the API; keep the same signature (runtime *common.RuntimeContext, appIDs []string, nameMap map[string]string) and ensure errors are handled/ignored consistently as before.
134-147: ⚡ Quick winThe assumption that bot/app sender
idis always anapp_idis correct per the API contract.The Lark IM v1 messages-list response guarantees that when
sender_typeis "app" or "bot",id_typewill be "app_id" andsender.idwill contain the application id—never anopen_idlikeou_.... The code is safe as written.That said, adding explicit validation (e.g., checking
id_type == "app_id"or validating the id prefix) would improve defensive consistency with the user branch and protect against unexpected edge cases.🤖 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/helpers.go` around lines 134 - 147, The switch handling senderType ("user"/"app"/"bot") assumes app/bot ids are always app IDs; add a defensive check in the "app" and "bot" branch to verify the id is an app ID (e.g., check id_type == "app_id" if that variable exists or assert the id does not have the "ou_" prefix) before marking seenApps and appending to missingAppIDs; if the validation fails, skip the id (similar to the user branch) so seenApps, missingAppIDs, seenUsers, and missingUserIDs updates remain consistent.
🤖 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.
Nitpick comments:
In `@shortcuts/im/convert_lib/helpers.go`:
- Around line 75-85: Update the ResolveSenderNames doc block to reflect the
current three-step behavior: Step 1 extracts display names from message
mentions, Step 2 batch-resolves user IDs via the contact API (requires
contact:user.base:readonly), and Step 3 resolves bot/app senders via the
application API (requires application:application:readonly); also note that
newly resolved names are written back into the provided cache map, and that the
function may return partial results if any API call fails.
- Around line 247-251: The fallback to data["app_name"] appears unreachable
given the v6 API nests app metadata under data["app"]; verify by checking
observed response shapes and, if no responses place app_name at the top level,
remove the fallback lines (the cast of data["app_name"] and the alternate
assignment) and rely on app["app_name"]; otherwise keep the fallback but add a
clarifying comment above this block stating which API versions or endpoints
produce a top-level app_name so future readers know it's intentional
(referencing the variables data, app and the app_name assignment in helpers.go).
- Around line 240-256: The function batchResolveApps is misleading because it
performs serial per-ID requests; either rename it to resolveAppsSerially (and
update all callers) or add limited concurrency: use an errgroup (or goroutine
pool) to issue doAPIJSONAsBotIfPossible calls concurrently for elements of
appIDs while protecting writes to nameMap with a mutex and bounding parallelism
(e.g., a semaphore channel or worker pool) to avoid overwhelming the API; keep
the same signature (runtime *common.RuntimeContext, appIDs []string, nameMap
map[string]string) and ensure errors are handled/ignored consistently as before.
- Around line 134-147: The switch handling senderType ("user"/"app"/"bot")
assumes app/bot ids are always app IDs; add a defensive check in the "app" and
"bot" branch to verify the id is an app ID (e.g., check id_type == "app_id" if
that variable exists or assert the id does not have the "ou_" prefix) before
marking seenApps and appending to missingAppIDs; if the validation fails, skip
the id (similar to the user branch) so seenApps, missingAppIDs, seenUsers, and
missingUserIDs updates remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac70bfa5-d62f-47ee-84d8-e2c63f632da1
📒 Files selected for processing (2)
shortcuts/im/convert_lib/helpers.goshortcuts/im/convert_lib/helpers_test.go
2 participants
As requested, this PR resolves bot sender names in
chat-messages-listand related commands by extracting app/bot IDs and querying their names via the Application OpenAPI.Summary by CodeRabbit
Release Notes