feat(im): add message_app_link to message outputs#668
feat(im): add message_app_link to message outputs#668sammi-bytedance wants to merge 1 commit intolarksuite:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds preservation and deterministic construction of 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
sammi-bytedance
force-pushed
the
feat/im-message-applink
branch
from
5ed0570 to
4cf23d0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/cli_e2e/im/message_get_workflow_test.go (1)
45-50: Assertchat_idagainst the created chat before link validation.Line 47-Line 49 currently pass
chat_idfrom the same response object into the helper, so this only checks internal consistency. Add an equality check against the knownchatIDto catch cross-chat mismatches.Suggested patch
requireChatMessageAppLink( - t, - messages[0].Get("message_app_link").String(), - messages[0].Get("chat_id").String(), - messages[0].Get("message_position").String(), + t, + messages[0].Get("message_app_link").String(), + func() string { + msgChatID := messages[0].Get("chat_id").String() + require.Equal(t, chatID, msgChatID, "stdout:\n%s", result.Stdout) + return msgChatID + }(), + messages[0].Get("message_position").String(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/message_get_workflow_test.go` around lines 45 - 50, The test currently only validates internal consistency by passing messages[0].Get("chat_id") into requireChatMessageAppLink; add an explicit equality assertion that messages[0].Get("chat_id").String() equals the known chatID before calling requireChatMessageAppLink so the test verifies the message belongs to the created chat (use the existing t and messages variables and perform a require.Equal or assert.Equal on chatID vs messages[0].Get("chat_id").String()).tests/cli_e2e/im/message_reply_workflow_test.go (1)
106-124: AvoidchatIDshadowing and assert against the known workflow chat.Line 106 shadows the outer
chatID, so current validation can pass even if the returned item belongs to the wrong chat but is self-consistent. Use a distinct variable and assert equality with the outerchatID.Suggested patch
- chatID := item.Get("chat_id").String() + itemChatID := item.Get("chat_id").String() + require.Equal(t, chatID, itemChatID, "stdout:\n%s", threadResult.Stdout) threadMsgPos := item.Get("thread_message_position").String() msgPos := item.Get("message_position").String() if appLink == "" { t.Fatalf("thread message_app_link is empty; chat_id=%q thread_id=%q thread_message_position=%q message_position=%q item=%s\nstdout:\n%s", - chatID, + itemChatID, item.Get("thread_id").String(), threadMsgPos, msgPos, item.Raw, threadResult.Stdout, ) } requireThreadMessageAppLink( t, appLink, threadID, - chatID, + itemChatID, threadMsgPos, )🤖 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 106 - 124, The inner variable chatID shadows the outer chatID causing false-positive validation; rename the inner extraction to a distinct name (e.g., itemChatID) when calling item.Get("chat_id").String(), add an assertion that itemChatID == chatID (the outer variable) before calling requireThreadMessageAppLink, and then pass itemChatID (or continue using the verified chatID) into requireThreadMessageAppLink to ensure the returned item belongs to the expected workflow chat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli_e2e/im/message_get_workflow_test.go`:
- Around line 45-50: The test currently only validates internal consistency by
passing messages[0].Get("chat_id") into requireChatMessageAppLink; add an
explicit equality assertion that messages[0].Get("chat_id").String() equals the
known chatID before calling requireChatMessageAppLink so the test verifies the
message belongs to the created chat (use the existing t and messages variables
and perform a require.Equal or assert.Equal on chatID vs
messages[0].Get("chat_id").String()).
In `@tests/cli_e2e/im/message_reply_workflow_test.go`:
- Around line 106-124: The inner variable chatID shadows the outer chatID
causing false-positive validation; rename the inner extraction to a distinct
name (e.g., itemChatID) when calling item.Get("chat_id").String(), add an
assertion that itemChatID == chatID (the outer variable) before calling
requireThreadMessageAppLink, and then pass itemChatID (or continue using the
verified chatID) into requireThreadMessageAppLink to ensure the returned item
belongs to the expected workflow chat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ce7224e-d7f0-4f64-9452-cac86405f885
📒 Files selected for processing (8)
internal/core/types.gointernal/core/types_test.goshortcuts/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
✅ Files skipped from review due to trivial changes (1)
- internal/core/types_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cli_e2e/im/chat_message_workflow_test.go
- shortcuts/im/convert_lib/content_convert.go
Preserve and assemble message_app_link in IM message formatting: - expose AppLink endpoint (applink.feishu.cn / applink.larksuite.com) via core.Endpoints, replacing the prior open.* host derivation - assemble URLs deterministically when the server omits the field: /client/thread/open for thread replies, /client/chat/open otherwise - normalize message_position across float64/int/json.Number/string - add unit and e2e coverage for chat/thread/fallback paths Change-Id: I98903b3a0136b8509415729beb28c86891ea5837
sammi-bytedance
force-pushed
the
feat/im-message-applink
branch
from
4cf23d0 to
aaf8b36
Compare
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
Summary by CodeRabbit
New Features
Tests