feat(im): add message_app_link to message outputs#668
feat(im): add message_app_link to message outputs#668sammi-bytedance wants to merge 4 commits intolarksuite:mainfrom
Conversation
Adds a convertlib-level resolver that maps brand to open-domain via ResolveEndpoints. Change-Id: I91fb000e6a04f82da08ccbb3ad80bff80fbf2e34
Change-Id: I67aac51e6f753201cef0ea0c5d96720dcec4040e
- Allow message_position/thread_message_position to be 0/negative as returned by IM APIs - Default unknown brand to feishu endpoints via ResolveEndpoints Change-Id: Ib532aeb1257d4bb138249e706e1d6750938f3775
Validate message_app_link structure for +messages-mget/+chat-messages-list and +threads-messages-list outputs. Change-Id: I58a6f437db4e5314e2dbae5783f925293336fe79
📝 WalkthroughWalkthroughThe changes add support for preserving and constructing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
github-actions
Bot
added
domain/im
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
shortcuts/im/convert_lib/content_convert.go (4)
197-216: Consider URL-escaping the path/query values when assemblingmessage_app_link.
chatID,threadID, and the normalized position strings are interpolated directly into the URL viafmt.Sprintfwithout URL-encoding. While IM IDs (oc_*,omt_*) and numeric positions are typically URL-safe in practice, building URLs vianet/url(e.g.,url.URL{Scheme, Host, Path}plusurl.Values) would be more defensive against unexpected characters and matches Go idioms for URL construction. Note the produced query parameter casing differs intentionally between the chat (openChatId/position) and thread (open_thread_id/open_chat_id/thread_position) endpoints, so aurl.Valuesmap preserves that.♻️ Optional refactor using net/url
func assembleMessageAppLink(m map[string]interface{}, brand core.LarkBrand) string { domain := resolveAppLinkDomain(brand) if domain == "" { return "" } chatID, _ := m["chat_id"].(string) threadID, _ := m["thread_id"].(string) msgPos, okMsgPos := normalizeMessagePosition(m["message_position"]) threadPos, okThreadPos := normalizeMessagePosition(m["thread_message_position"]) - // Thread app link requires both thread_id and chat_id. - if threadID != "" && chatID != "" && okThreadPos { - return fmt.Sprintf("https://%s/client/thread/open?open_thread_id=%s&open_chat_id=%s&thread_position=%s", domain, threadID, chatID, threadPos) - } - if chatID != "" && okMsgPos { - return fmt.Sprintf("https://%s/client/chat/open?openChatId=%s&position=%s", domain, chatID, msgPos) - } - return "" + u := url.URL{Scheme: "https", Host: domain} + q := url.Values{} + switch { + case threadID != "" && chatID != "" && okThreadPos: + u.Path = "/client/thread/open" + q.Set("open_thread_id", threadID) + q.Set("open_chat_id", chatID) + q.Set("thread_position", threadPos) + case chatID != "" && okMsgPos: + u.Path = "/client/chat/open" + q.Set("openChatId", chatID) + q.Set("position", msgPos) + default: + return "" + } + u.RawQuery = q.Encode() + return u.String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/convert_lib/content_convert.go` around lines 197 - 216, The assembleMessageAppLink function interpolates chatID, threadID, msgPos and threadPos directly into the URL which can risk unescaped characters; change it to build the link with net/url (use url.URL{Scheme:"https", Host: domain, Path: ...} and url.Values for query params) and set the query parameters with the exact casing used currently ("open_thread_id","open_chat_id","thread_position" for thread links and "openChatId","position" for chat links) so values are URL-encoded; update the thread and chat branches in assembleMessageAppLink to construct and return url.String() rather than using fmt.Sprintf.
263-270:resolveAppLinkDomainswallows emptyHostfrom a parsed URL.
url.Parsereturns no error for many malformed inputs (e.g., an empty string,"feishu.cn"with no scheme), in which caseu.Hostmay be empty and the assembler will quietly return"". This is fine ifcore.ResolveEndpoints(brand).Openis guaranteed to be a well-formedhttps://host[/...]URL for bothBrandFeishu/BrandLarkand the unknown-brand fallback (the unit tests confirm this forfeishu/lark/other). Worth a brief comment documenting the contract, or a fallback to a literal default (e.g.,open.feishu.cn) on emptyHost, so a future endpoint config regression doesn't silently disablemessage_app_linkeverywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/convert_lib/content_convert.go` around lines 263 - 270, resolveAppLinkDomain currently returns u.Host even when url.Parse succeeds but yields an empty Host (e.g., no scheme), which can silently break message_app_link; update resolveAppLinkDomain to check if u == nil or u.Host is empty after parsing core.ResolveEndpoints(brand).Open and, if empty, return a safe fallback host (for example "open.feishu.cn") or derive a sensible default based on brand, and add a brief comment documenting the expectation that ResolveEndpoints(...).Open should be a full https:// URL; reference the resolveAppLinkDomain function and core.ResolveEndpoints(brand).Open when making the change.
218-261:normalizeMessagePositiondoesn't coverint32/uint*numeric types.The type switch handles
float64,int,int64,json.Number, andstring, but notint32,uint,uint32,uint64. JSON unmarshalling intomap[string]interface{}usesfloat64(orjson.NumberwhenUseNumberis set), so this is unlikely to manifest in the current call path. However, if a caller ever feeds in a value from a typed struct or a different decoder, those numeric kinds will silently fall through to thedefaultbranch and the link won't be assembled. Consider adding the additional integer cases (or usereflect) for resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/convert_lib/content_convert.go` around lines 218 - 261, The normalizeMessagePosition function's type switch misses other integer kinds (e.g., int32, uint, uint32, uint64), causing them to hit the default false path; update the type switch inside normalizeMessagePosition to add cases for int8/int16/int32, uint/uint8/uint16/uint32/uint64 (and uintptr if desired) and convert those to a canonical integer representation (int64 or uint64) then format with strconv.FormatInt/FormatUint (or convert unsigned to signed only when safe) so they return the same string,true behavior as int/int64; alternatively, replace the type-switch branch for numeric types with a reflect-based check that accepts all integer kinds and floats and normalizes them the same way.
156-177: Subtle interaction between field preservation and link assembly.Lines 166-168 unconditionally copy
raw["message_app_link"](any underlying type) intomsg, then lines 171-177 may overwrite it only when the type-assertion tostringyields an empty string. If the server ever returnsmessage_app_linkas a non-string (unlikely but defensible), the preserved non-string value will be silently replaced by an assembled string, which is inconsistent with the intended “pass-through if server provided one” semantics.Consider gating preservation through the same string type-assertion used by the assembler, so the two branches stay aligned:
♻️ Tighter pass-through gate
- if v, ok := m["message_app_link"]; ok { - msg["message_app_link"] = v - } - - // Assemble message_app_link deterministically when server doesn't provide one. - if runtime != nil && runtime.Config != nil { - if rawAppLink, _ := m["message_app_link"].(string); rawAppLink == "" { - if assembled := assembleMessageAppLink(m, runtime.Config.Brand); assembled != "" { - msg["message_app_link"] = assembled - } - } - } + rawAppLink, _ := m["message_app_link"].(string) + if rawAppLink != "" { + msg["message_app_link"] = rawAppLink + } else if runtime != nil && runtime.Config != nil { + if assembled := assembleMessageAppLink(m, runtime.Config.Brand); assembled != "" { + msg["message_app_link"] = assembled + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/convert_lib/content_convert.go` around lines 156 - 177, The current code copies m["message_app_link"] into msg unconditionally which can conflict with assembleMessageAppLink; change the preservation to the same string type-assertion/empty check used later: when handling the "message_app_link" field, only copy it into msg if you can assert it to a non-empty string (use the same rawAppLink, _ := m["message_app_link"].(string) pattern), otherwise leave it unset so assembleMessageAppLink(m, runtime.Config.Brand) can deterministically populate it; update the block that currently references m["message_app_link"], msg, runtime, and assembleMessageAppLink to follow this gated behavior.shortcuts/im/convert_lib/content_media_misc_test.go (1)
66-212: Solid coverage for the newmessage_app_linkandnormalizeMessagePositionpaths.The added tests exercise pass-through, chat/thread assembly, brand fallback, nil-runtime no-op, missing-fields no-op, invalid-thread-position fallback, and zero/negative numeric positions across both numeric and string inputs. Two small optional gaps if you want belt-and-suspenders coverage:
json.Numberinput tonormalizeMessagePosition(thecase json.Number:branch isn't directly exercised — current tests rely onfloat64/string).- An integer-valued float case (
float64(12.0)) is exercised indirectly inAssembleChat, but a direct unit assertion againstnormalizeMessagePositionformath.Trunc(f) == fformatting could make the contract explicit.Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/convert_lib/content_media_misc_test.go` around lines 66 - 212, Add two small unit tests to exercise the json.Number and integer-valued float branches of normalizeMessagePosition: call normalizeMessagePosition(json.Number("12")) and assert it returns "12", and call normalizeMessagePosition(float64(12.0)) and assert it returns "12"; place these alongside the existing TestNormalizeMessagePosition_AllowsZeroAndNegative tests so the json.Number and the integer float (math.Trunc(f) == f) code paths in normalizeMessagePosition are covered.tests/cli_e2e/im/message_reply_workflow_test.go (1)
105-126: Empty-linkt.Fatalfis redundant withrequireThreadMessageAppLink's non-empty assertion.
requireThreadMessageAppLinkalready callsrequire.NotEmpty(t, appLink, ...)first thing, so theif appLink == ""block effectively duplicates that check. The motivation appears to be richer diagnostics (the explicit field dump anditem.Raw), which is understandable. If you want to keep both, fine — otherwise consider folding the diagnostic into the helper (e.g., as an additionalmsgAndArgspayload) so failure reporting lives in one place.Also, the trailing
require.Equal(t, threadID, item.Get("thread_id").String(), ...)on Line 126 is partially covered by the helper (which asserts the URL'sopen_thread_idequals the passedthreadID); it's still a useful end-to-end self-consistency check between the URL and the response field, so leaving it is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/message_reply_workflow_test.go` around lines 105 - 126, The explicit empty-link t.Fatalf block duplicates the non-empty assertion already performed by requireThreadMessageAppLink; remove the if appLink == "" { t.Fatalf(...) } block and rely on requireThreadMessageAppLink(t, appLink, threadID, chatID, threadMsgPos) instead, or alternatively move the rich diagnostic payload (item.Raw, threadResult.Stdout and field values) into requireThreadMessageAppLink so the helper logs those details when it calls require.NotEmpty; keep the trailing require.Equal(t, threadID, item.Get("thread_id").String(), ...) as the end-to-end consistency check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/im/convert_lib/content_convert.go`:
- Around line 197-216: The assembleMessageAppLink function interpolates chatID,
threadID, msgPos and threadPos directly into the URL which can risk unescaped
characters; change it to build the link with net/url (use
url.URL{Scheme:"https", Host: domain, Path: ...} and url.Values for query
params) and set the query parameters with the exact casing used currently
("open_thread_id","open_chat_id","thread_position" for thread links and
"openChatId","position" for chat links) so values are URL-encoded; update the
thread and chat branches in assembleMessageAppLink to construct and return
url.String() rather than using fmt.Sprintf.
- Around line 263-270: resolveAppLinkDomain currently returns u.Host even when
url.Parse succeeds but yields an empty Host (e.g., no scheme), which can
silently break message_app_link; update resolveAppLinkDomain to check if u ==
nil or u.Host is empty after parsing core.ResolveEndpoints(brand).Open and, if
empty, return a safe fallback host (for example "open.feishu.cn") or derive a
sensible default based on brand, and add a brief comment documenting the
expectation that ResolveEndpoints(...).Open should be a full https:// URL;
reference the resolveAppLinkDomain function and
core.ResolveEndpoints(brand).Open when making the change.
- Around line 218-261: The normalizeMessagePosition function's type switch
misses other integer kinds (e.g., int32, uint, uint32, uint64), causing them to
hit the default false path; update the type switch inside
normalizeMessagePosition to add cases for int8/int16/int32,
uint/uint8/uint16/uint32/uint64 (and uintptr if desired) and convert those to a
canonical integer representation (int64 or uint64) then format with
strconv.FormatInt/FormatUint (or convert unsigned to signed only when safe) so
they return the same string,true behavior as int/int64; alternatively, replace
the type-switch branch for numeric types with a reflect-based check that accepts
all integer kinds and floats and normalizes them the same way.
- Around line 156-177: The current code copies m["message_app_link"] into msg
unconditionally which can conflict with assembleMessageAppLink; change the
preservation to the same string type-assertion/empty check used later: when
handling the "message_app_link" field, only copy it into msg if you can assert
it to a non-empty string (use the same rawAppLink, _ :=
m["message_app_link"].(string) pattern), otherwise leave it unset so
assembleMessageAppLink(m, runtime.Config.Brand) can deterministically populate
it; update the block that currently references m["message_app_link"], msg,
runtime, and assembleMessageAppLink to follow this gated behavior.
In `@shortcuts/im/convert_lib/content_media_misc_test.go`:
- Around line 66-212: Add two small unit tests to exercise the json.Number and
integer-valued float branches of normalizeMessagePosition: call
normalizeMessagePosition(json.Number("12")) and assert it returns "12", and call
normalizeMessagePosition(float64(12.0)) and assert it returns "12"; place these
alongside the existing TestNormalizeMessagePosition_AllowsZeroAndNegative tests
so the json.Number and the integer float (math.Trunc(f) == f) code paths in
normalizeMessagePosition are covered.
In `@tests/cli_e2e/im/message_reply_workflow_test.go`:
- Around line 105-126: The explicit empty-link t.Fatalf block duplicates the
non-empty assertion already performed by requireThreadMessageAppLink; remove the
if appLink == "" { t.Fatalf(...) } block and rely on
requireThreadMessageAppLink(t, appLink, threadID, chatID, threadMsgPos) instead,
or alternatively move the rich diagnostic payload (item.Raw, threadResult.Stdout
and field values) into requireThreadMessageAppLink so the helper logs those
details when it calls require.NotEmpty; keep the trailing require.Equal(t,
threadID, item.Get("thread_id").String(), ...) as the end-to-end consistency
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f304490c-57c2-470b-8fb6-9450ca8f87ee
📒 Files selected for processing (6)
shortcuts/im/convert_lib/content_convert.goshortcuts/im/convert_lib/content_media_misc_test.gotests/cli_e2e/im/chat_message_workflow_test.gotests/cli_e2e/im/helpers_test.gotests/cli_e2e/im/message_get_workflow_test.gotests/cli_e2e/im/message_reply_workflow_test.go
1 participant
Summary
IM message-related shortcuts now return a deterministic deep link (
message_app_link) assembled from message fields when the server does not provide one.Changes
message_app_linkfor chat and thread messages inshortcuts/im/convert_lib/FormatMessageItem.chat_id,message_position,thread_message_position, and existingmessage_app_linkfields.message_position/thread_message_positionto be 0 or negative (as returned by IM APIs) when assembling the link.+messages-mget,+chat-messages-list, and+threads-messages-list.Test Plan
go test ./shortcuts/im/convert_lib -count=1go test -v ./tests/cli_e2e/im -run TestIM_MessageReplyWorkflowAsBot -count=1go test -v ./tests/cli_e2e/im -run 'TestIM_(MessageGetWorkflowAsUser|ChatMessageWorkflowAsUser)$' -count=1(skips without user token)Related Issues