feat(mail): support sharing emails to IM chats#637
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 flags (message_id xor thread_id, receive_id/type, mailbox)
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 Possibly related PRs
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 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@b8697bcbfc2c6549b4f12e82645f1c483154727f🧩 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 62.98% 62.99%
=======================================
Files 428 429 +1
Lines 37327 37387 +60
=======================================
+ Hits 23512 23551 +39
- Misses 11703 11723 +20
- Partials 2112 2113 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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
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
infeng
left a comment
infeng
left a comment
There was a problem hiding this comment.
LGTM. Prior AI review thread resolved after re-checking the current head 097b716288bd3548e7a4682dfd98a3a6aaac5a6b.
Review baseline: base main, head worktree-feat+mail-share-to-chat.
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
- 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
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
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
Change-Id: I47dc6a9a47252dcfb7853737f88dfdaef65a0ae7
Change-Id: I9f4a1a183b55d03f5248eb4adddfddb08037ca95
chanthuang
force-pushed
the
worktree-feat+mail-share-to-chat
branch
from
cd7b849 to
b8697bc
Compare
infeng
left a comment
infeng
left a comment
There was a problem hiding this comment.
LGTM. Re-checked the current head b8697bcbfc2c6549b4f12e82645f1c483154727f; the only follow-up change after the previous review is the dry-run API-count assertion, and the share-to-chat test suite still passes on the current tree.
Review baseline: base main, head worktree-feat+mail-share-to-chat.
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 an email message or thread to Lark IM as an interactive card; supports message-id or thread-id and multiple recipient ID types.