feat(mail): support sharing emails to IM chats#637
Conversation
Two-step API (create share token → send card) wrapped in a single shortcut. Supports message-id/thread-id, five receive-id-type variants (chat_id, open_id, user_id, union_id, email), and dry-run mode. Change-Id: Ic7b8c01c0d25fef262f35be92555f1fd019bd679 Co-Authored-By: AI
…edit Add missing safety rule 8 (draft link rule) to skill-template/domains/mail.md so it survives regeneration. SKILL.md is now produced by `make gen-skills` in the registry repo rather than hand-edited. Change-Id: I9cf3605deae8b6de2042e40819fedc304967e78e Co-Authored-By: AI
📝 WalkthroughWalkthroughAdds a new MailShareToChat CLI shortcut ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Validator as Validator
participant MailAPI as Mail Service
participant IMAPI as IM Service
CLI->>Validator: invoke +share-to-chat (flags)
Validator->>Validator: validate (message_id xor thread_id, receive_id/type)
alt validation fails
Validator-->>CLI: return validation error
else
Validator->>MailAPI: POST /mailboxes/{mailbox}/share_token (message_id|thread_id)
MailAPI-->>Validator: { card_id }
Validator->>IMAPI: POST /im/messages/send (card_id, receive_id, receive_id_type)
IMAPI-->>Validator: { im_message_id }
Validator-->>CLI: output { card_id, im_message_id }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 docstrings
🧪 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/mail
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/mail/mail_share_to_chat_test.go (2)
103-114: Replace hand-rolledcontainsStr/searchStrwithstrings.Contains.Standard library already provides this; the custom implementation adds noise and an unnecessary
len(s) >= len(substr)guard (strings.Containsalready handles all cases).♻️ Proposed diff
import ( "fmt" + "strings" "testing" )- if !containsStr(err.Error(), tt.wantErr) { + if !strings.Contains(err.Error(), tt.wantErr) { t.Fatalf("expected error containing %q, got %q", tt.wantErr, err.Error()) }-func containsStr(s, substr string) bool { - return len(s) >= len(substr) && searchStr(s, substr) -} - -func searchStr(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_share_to_chat_test.go` around lines 103 - 114, The file defines custom containsStr and searchStr functions; replace their usage with the stdlib strings.Contains and delete these two functions: change any call sites using containsStr(...) to strings.Contains(...), remove searchStr entirely, and add "strings" to the imports if not already present so the tests compile.
54-101: Tests duplicate validation logic instead of exercisingMailShareToChat.Validate.
validateShareToChatis a parallel reimplementation of the real validation inmail_share_to_chat.go. If someone later changes the productionValidate(e.g., adds a receive-id-type, tightens a rule, or modifies the error wrapping withoutput.ErrValidation), these tests will still pass while the shipped behavior silently diverges. The only thing currently covered end-to-end isvalidReceiveIDTypes(inTestValidReceiveIDTypes).Additionally,
containsStr()andsearchStr()reinventstrings.Contains().Consider rewriting to drive
MailShareToChat.Validatethrough a real*common.RuntimeContext(as other shortcut tests in this package do viacmdutil.TestFactoryandrunMountedMailShortcut) so assertions exercise the production code path, theoutput.ErrValidationwrapping, and avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_share_to_chat_test.go` around lines 54 - 101, The test duplicates validation logic via validateShareToChat; replace those table-driven calls with exercising the real MailShareToChat.Validate path using a runtime context (e.g., create a MailShareToChat instance and call its Validate with a *common.RuntimeContext built via cmdutil.TestFactory and runMountedMailShortcut like other shortcuts in this package), remove the validateShareToChat helper, and assert errors using strings.Contains on the returned error (verifying output.ErrValidation wrapping where appropriate) instead of the custom containsStr/searchStr helpers so the unit test follows production validation logic.
🤖 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/mail/mail_share_to_chat_test.go`:
- Around line 103-114: The file defines custom containsStr and searchStr
functions; replace their usage with the stdlib strings.Contains and delete these
two functions: change any call sites using containsStr(...) to
strings.Contains(...), remove searchStr entirely, and add "strings" to the
imports if not already present so the tests compile.
- Around line 54-101: The test duplicates validation logic via
validateShareToChat; replace those table-driven calls with exercising the real
MailShareToChat.Validate path using a runtime context (e.g., create a
MailShareToChat instance and call its Validate with a *common.RuntimeContext
built via cmdutil.TestFactory and runMountedMailShortcut like other shortcuts in
this package), remove the validateShareToChat helper, and assert errors using
strings.Contains on the returned error (verifying output.ErrValidation wrapping
where appropriate) instead of the custom containsStr/searchStr helpers so the
unit test follows production validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08b5827d-e48d-41ab-8d96-7c9ea20fb938
📒 Files selected for processing (6)
shortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_share_to_chat_test.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-share-to-chat.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@931b5c3ace955ddcf09d953dad3e1cb3ff9cd7f2🧩 Skill updateUnavailable for this PR because source repo/branch metadata is missing. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 60.37% 60.69% +0.31%
==========================================
Files 408 417 +9
Lines 43411 44046 +635
==========================================
+ Hits 26210 26732 +522
- Misses 15159 15243 +84
- Partials 2042 2071 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add Go doc comments to exported symbols for docstring coverage - Rewrite tests to exercise MailShareToChat.Validate via RuntimeContext instead of duplicating validation logic - Replace hand-rolled containsStr with strings.Contains - Add httpmock stubs for execute and error path tests Change-Id: Ic781494f61e9e844224185844bce7b0c48e8e200 Co-Authored-By: AI
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_share_to_chat_test.go`:
- Around line 88-121: The test TestShareToChatExecuteWithThreadID only checks
for card_002 and doesn't verify om_002 or that the outgoing request used
thread_id, so update the test to assert the response contains "om_002" (same as
the message-id test) and enhance the second httpmock.Stub (the POST to
"/user_mailboxes/me/share_tokens/card_002/send") to capture and assert the
request body includes "thread_id":"t1" (use the httpmock request-capture
facility) to detect regressions where Execute might send message_id instead of
thread_id; keep references to TestShareToChatExecuteWithThreadID,
runMountedMailShortcut, and MailShareToChat when making these changes.
- Around line 1-190: The PR is missing the required dry-run E2E test for the new
MailShareToChat shortcut; add a dry-run E2E test under tests/cli_e2e/dryrun/
that runs the CLI with MailShareToChat and the --dry-run flag while setting
LARKSUITE_CLI_APP_ID, APP_SECRET and BRAND env vars, and assert the generated
request payloads for the create-share-token (messages/share_token) and send-card
(share_tokens/{card_id}/send) calls match the expected structure; implement the
test so it invokes the same command-line invocation used in unit tests (e.g.,
"+share-to-chat" with --message-id/--thread-id and
--receive-id/--receive-id-type) and validates both requests are produced without
actually calling the network.
🪄 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: ec6a2c18-7850-4a20-b46b-bf917087a193
📒 Files selected for processing (2)
shortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_share_to_chat_test.go
Validate request shape (method, URL, mailbox path) under --dry-run with fake credentials. Covers message-id, thread-id, and custom mailbox variants. Change-Id: Iae87bf141cbe4f312d3e9b1fca4ba175052c5c35 Co-Authored-By: AI
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go (1)
89-91: Preferstrconv.Itoa(i)overstring(rune('0'+i))for the gjson index.The
string(rune('0'+i))trick silently produces non-digit runes oncei >= 10, which would cause confusinggjsonlookups returning empty strings rather than a clear test failure. Since the current test only iterates 2 URLs it works, but it's a footgun for anyone extendingwantURLs. Usingstrconv.Itoa(orfmt.Sprintf) is clearer and future-proof.♻️ Proposed refactor
out := result.Stdout for i, wantURL := range tt.wantURLs { - gotMethod := gjson.Get(out, "api."+string(rune('0'+i))+".method").String() - gotURL := gjson.Get(out, "api."+string(rune('0'+i))+".url").String() + idx := strconv.Itoa(i) + gotMethod := gjson.Get(out, "api."+idx+".method").String() + gotURL := gjson.Get(out, "api."+idx+".url").String()And add
"strconv"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go` around lines 89 - 91, The test uses string(rune('0'+i)) to build gjson paths which breaks for i>=10; replace string(rune('0'+i)) with strconv.Itoa(i) when constructing the gjson key in the loop that reads wantURLs and sets gotMethod/gotURL (the gjson.Get calls), and add "strconv" to the imports so multi-digit indices produce correct JSON lookups and clear failures.
🤖 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/mail/mail_share_to_chat_dryrun_test.go`:
- Around line 89-91: The test uses string(rune('0'+i)) to build gjson paths
which breaks for i>=10; replace string(rune('0'+i)) with strconv.Itoa(i) when
constructing the gjson key in the loop that reads wantURLs and sets
gotMethod/gotURL (the gjson.Get calls), and add "strconv" to the imports so
multi-digit indices produce correct JSON lookups and clear failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e190ed06-955b-4222-b24e-7053a3fffc03
📒 Files selected for processing (1)
tests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go
| return common.NewDryRunAPI(). | ||
| Desc("Share email card: create share token → send card to IM chat"). | ||
| POST(mailboxPath(mailboxID, "messages", "share_token")). | ||
| POST(mailboxPath(mailboxID, "share_tokens", "<card_id>", "send")) |
There was a problem hiding this comment.
Issue: --dry-run currently previews only the two endpoint URLs, but not the payload/query values that actually distinguish the execution path. In particular, the first POST does not show whether the command will create a share token from message_id or thread_id, and the second POST does not expose receive_id_type / receive_id. That makes the new dry-run mode much less trustworthy for callers, and regressions in those fields will never show up in the dry-run output even if the E2E test passes on URLs alone.
Severity: 🚨 Should Fix
Suggested Fix: Keep DryRun in lockstep with Execute: attach the first request body ({"message_id": ...} or {"thread_id": ...}) to the share-token POST, and attach both receive_id_type and {"receive_id": ...} to the send POST. Then extend the dry-run test to assert those fields as well, not just the URLs.
There was a problem hiding this comment.
Fixed in 931b5c3. DryRun now includes request body and params for both API calls:
- POST share_token:
body: {message_id: ...}or{thread_id: ...} - POST send:
params: {receive_id_type: ...},body: {receive_id: ...}
E2E dry-run test updated to assert these fields.
DryRun now mirrors Execute: the share-token POST shows message_id or thread_id, and the send POST shows receive_id_type and receive_id. E2E test updated to assert these fields. Also fix strconv.Itoa usage. Change-Id: I00f8770fd5a12b7354986c5e5077f97cfe5d6653
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go`:
- Around line 101-112: Add an explicit assertion that the dry-run emitted
exactly len(tt.wantURLs) API calls before iterating: compute the number of
entries in the JSON API array from result.Stdout (e.g. use gjson.Get(out,
"api").Array() or gjson.Get(out, "api.#") to get the count), compare it to
len(tt.wantURLs), and fail the test if they differ; place this check just before
the existing for-loop that uses out, tt.wantURLs, and gjson.Get so any
unexpected extra/missing calls are caught.
🪄 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: eae887e6-ccef-4ce5-9f48-d481f5bd7e08
📒 Files selected for processing (2)
shortcuts/mail/mail_share_to_chat.gotests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go
| out := result.Stdout | ||
| for i, wantURL := range tt.wantURLs { | ||
| idx := strconv.Itoa(i) | ||
| gotMethod := gjson.Get(out, "api."+idx+".method").String() | ||
| gotURL := gjson.Get(out, "api."+idx+".url").String() | ||
| if gotMethod != "POST" { | ||
| t.Fatalf("api[%d].method = %q, want POST\nstdout:\n%s", i, gotMethod, out) | ||
| } | ||
| if gotURL != wantURL { | ||
| t.Fatalf("api[%d].url = %q, want %q\nstdout:\n%s", i, gotURL, wantURL, out) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert the dry-run emits exactly two API calls.
The loop verifies the first two entries, but a regression that adds an unexpected third dry-run request would still pass. Add an API-count assertion before checking the entries.
Proposed test hardening
out := result.Stdout
+ gotAPICount := int(gjson.Get(out, "api.#").Int())
+ if gotAPICount != len(tt.wantURLs) {
+ t.Fatalf("api count = %d, want %d\nstdout:\n%s", gotAPICount, len(tt.wantURLs), out)
+ }
for i, wantURL := range tt.wantURLs {
idx := strconv.Itoa(i)
gotMethod := gjson.Get(out, "api."+idx+".method").String()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out := result.Stdout | |
| for i, wantURL := range tt.wantURLs { | |
| idx := strconv.Itoa(i) | |
| gotMethod := gjson.Get(out, "api."+idx+".method").String() | |
| gotURL := gjson.Get(out, "api."+idx+".url").String() | |
| if gotMethod != "POST" { | |
| t.Fatalf("api[%d].method = %q, want POST\nstdout:\n%s", i, gotMethod, out) | |
| } | |
| if gotURL != wantURL { | |
| t.Fatalf("api[%d].url = %q, want %q\nstdout:\n%s", i, gotURL, wantURL, out) | |
| } | |
| } | |
| out := result.Stdout | |
| gotAPICount := int(gjson.Get(out, "api.#").Int()) | |
| if gotAPICount != len(tt.wantURLs) { | |
| t.Fatalf("api count = %d, want %d\nstdout:\n%s", gotAPICount, len(tt.wantURLs), out) | |
| } | |
| for i, wantURL := range tt.wantURLs { | |
| idx := strconv.Itoa(i) | |
| gotMethod := gjson.Get(out, "api."+idx+".method").String() | |
| gotURL := gjson.Get(out, "api."+idx+".url").String() | |
| if gotMethod != "POST" { | |
| t.Fatalf("api[%d].method = %q, want POST\nstdout:\n%s", i, gotMethod, out) | |
| } | |
| if gotURL != wantURL { | |
| t.Fatalf("api[%d].url = %q, want %q\nstdout:\n%s", i, gotURL, wantURL, out) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/mail/mail_share_to_chat_dryrun_test.go` around lines 101 - 112,
Add an explicit assertion that the dry-run emitted exactly len(tt.wantURLs) API
calls before iterating: compute the number of entries in the JSON API array from
result.Stdout (e.g. use gjson.Get(out, "api").Array() or gjson.Get(out, "api.#")
to get the count), compare it to len(tt.wantURLs), and fail the test if they
differ; place this check just before the existing for-loop that uses out,
tt.wantURLs, and gjson.Get so any unexpected extra/missing calls are caught.
2 participants
Summary
Add
+share-to-chatshortcut that shares an email or thread as a card to a Lark IM chat (group or individual).chat_id,open_id,user_id,union_id,emailUsage
Test plan
Summary by CodeRabbit
+share-to-chatshortcut to share email messages or threads to Lark IM as interactive cards