Skip to content

feat: append default mail signatures on send#1332

Open
bubbmon233 wants to merge 1 commit into
larksuite:mainfrom
bubbmon233:feat/26bb14a
Open

feat: append default mail signatures on send#1332
bubbmon233 wants to merge 1 commit into
larksuite:mainfrom
bubbmon233:feat/26bb14a

Conversation

@bubbmon233

@bubbmon233 bubbmon233 commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Generated by the harness-coding skill.

  • Branch: feat/26bb14a
  • Target: main

Sprints

ID Title Status Commit
S1 Enhance mail +send default signature composition passed 8003378
S2 Synthesize transport contract for larksuite/cli passed 0d84751

Source specs

  • input/tech-design.md

This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.

Summary by CodeRabbit

  • New Features

    • +send shortcut now appends the sender's default signature by default.
    • Added --no-signature flag to skip signature appending.
    • Added --signature-id flag to explicitly select a specific signature.
    • Plain-text emails preserve text/plain format when signatures are appended.
  • Documentation

    • Updated +send shortcut documentation to explain signature behavior and new flags.

Resolve the sender's default USER signature during mail +send and keep plain-text sends text/plain by rendering signature HTML to text. Users can pass --no-signature to skip the default lookup.

sprint: S1
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds signature support to the mail +send shortcut. By default, signatures matching the sender's email are appended to outgoing messages. Users can skip signatures with --no-signature or override with --signature-id. Plain-text sends preserve the text/plain format while still appending the signature as text.

Changes

Mail Send Signature Support

Layer / File(s) Summary
Signature composition library
shortcuts/mail/signature_compose.go, shortcuts/mail/signature_compose_test.go
Refactored resolveSignature to support conditional image inclusion. Added resolveSignatureForSend to handle --no-signature flag, explicit signature override, and default selection by sender email matching. Introduced helpers for signature ID validation, default selection logic, and plain-text signature rendering.
Public API export
shortcuts/mail/draft/htmltext.go
Exported PlainTextFromHTML as public API to expose HTML-to-plaintext conversion outside the package.
Mail send CLI integration
shortcuts/mail/mail_send.go
Integrated --no-signature flag into CLI. Conditional dry-run planning fetches signature data only when signatures enabled. Updated validation to enforce mutual exclusivity of signature options. Applied resolved signatures to message bodies via plain-text appending.
Integration tests
shortcuts/mail/mail_send_signature_test.go
Comprehensive tests validating default signature appending in HTML mode, plain-text mode preserving text/plain, --no-signature skipping resolution, --signature-id override behavior, API failure graceful degradation with warning, and dry-run endpoint selection.
User-facing documentation
skill-template/domains/mail.md, skills/lark-mail/SKILL.md, skills/lark-mail/references/lark-mail-send.md
Updated mail domain template and skill documentation describing default signature appending, --no-signature and --signature-id usage, plain-text mode behavior, and signature selection rules by sender email.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • larksuite/cli#642: Both PRs modify the +send shortcut's message composition logic in mail_send.go (plainText and HTML body handling), so the new signature appending and template merge features interact in the same code paths.

Suggested labels

enhancement, domain/mail, size/M

Suggested reviewers

  • chanthuang
  • infeng

Poem

🐰 Signatures bloom where messages flow,
A personal touch, from head to toe.
Skip them with flags, or choose your own,
Plain-text stays plain, the best it's known!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description deviates significantly from the required template, missing critical sections like Summary, Changes list, and Test Plan with checkboxes. Restructure the description to follow the repository template: add a Summary section, list main changes (including new flags, test suite, and documentation updates), and specify which tests were run and manual verification steps.
Docstring Coverage ⚠️ Warning Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title 'feat: append default mail signatures on send' accurately and clearly describes the main change: adding support for appending default mail signatures when sending emails.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added domain/mail PR touches the mail domain size/M Single-domain feat or fix with limited business impact labels Jun 8, 2026
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.00000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.56%. Comparing base (0d84751) to head (8003378).

Files with missing lines Patch % Lines
shortcuts/mail/signature_compose.go 71.08% 17 Missing and 7 partials ⚠️
shortcuts/mail/draft/htmltext.go 0.00% 2 Missing ⚠️
shortcuts/mail/mail_send.go 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1332      +/-   ##
==========================================
+ Coverage   71.47%   71.56%   +0.08%     
==========================================
  Files         688      688              
  Lines       65482    65560      +78     
==========================================
+ Hits        46806    46918     +112     
+ Misses      15031    14984      -47     
- Partials     3645     3658      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8003378c68a39418c1b06f2af5c6824dbf12b869

🧩 Skill update

npx skills add bubbmon233/cli#feat/26bb14a -y -g

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
shortcuts/mail/mail_send_signature_test.go (1)

49-89: ⚡ Quick win

Add a happy-path test for auto-detected plain bodies.

The suite verifies --plain-text and the fail-open fallback, but it never asserts the new success path where --body "Hello" without --plain-text appends a text-converted signature. A small case for that branch would lock down the behavior added in mail_send.go.

Also applies to: 155-186

🤖 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/mail/mail_send_signature_test.go` around lines 49 - 89, Add a new
happy-path test that exercises the auto-detection branch when the user provides
--body "Hello" without --plain-text: create a test (e.g.,
TestMailSendAutoDetectPlainBodyAppendsTextSignature) using the same
fixtures/helpers used in
TestMailSendPlainTextDefaultSignatureKeepsTextPlain—call
mailShortcutTestFactoryWithSendScope, registerSignatureListStub (with a USER
signature containing HTML content), registerSendAsStub, registerDraftCreateStub,
then run runMountedMailShortcut with MailSend and arguments including "--body",
"Hello" (but omit "--plain-text"); assert the drafted EML (via
mustDecodeRawEMLFromStub) contains Content-Type: text/plain and that the
text-converted signature text (e.g., "Regards" and name) is appended, and that
no HTML parts or "lark-mail-signature" are present so the auto-detect plain path
is locked down.
shortcuts/mail/signature_compose.go (1)

164-170: Document the "0" sentinel meaning for signature IDs

normalizeSignatureID treats "0" as empty (returns "" when signatureID == "0"), and default-selection code/tests also ignore "0" as an unset ID; the current search found no supporting inline docs/comments for this convention, so add a clarifying comment or named constant.

🤖 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/mail/signature_compose.go` around lines 164 - 170,
normalizeSignatureID currently treats the string "0" as a sentinel for an unset
signature but has no documentation; update the code by introducing a named
constant (e.g., unsetSignatureID = "0") and/or add a concise inline comment
above the normalizeSignatureID function explaining that "0" is treated as an
unset/placeholder ID and should be treated the same as an empty string, then use
that constant in normalizeSignatureID (and update any related default-selection
code/tests to reference the constant) so the convention is explicit and
discoverable.
🤖 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/mail/mail_send_signature_test.go`:
- Around line 49-89: Add a new happy-path test that exercises the auto-detection
branch when the user provides --body "Hello" without --plain-text: create a test
(e.g., TestMailSendAutoDetectPlainBodyAppendsTextSignature) using the same
fixtures/helpers used in
TestMailSendPlainTextDefaultSignatureKeepsTextPlain—call
mailShortcutTestFactoryWithSendScope, registerSignatureListStub (with a USER
signature containing HTML content), registerSendAsStub, registerDraftCreateStub,
then run runMountedMailShortcut with MailSend and arguments including "--body",
"Hello" (but omit "--plain-text"); assert the drafted EML (via
mustDecodeRawEMLFromStub) contains Content-Type: text/plain and that the
text-converted signature text (e.g., "Regards" and name) is appended, and that
no HTML parts or "lark-mail-signature" are present so the auto-detect plain path
is locked down.

In `@shortcuts/mail/signature_compose.go`:
- Around line 164-170: normalizeSignatureID currently treats the string "0" as a
sentinel for an unset signature but has no documentation; update the code by
introducing a named constant (e.g., unsetSignatureID = "0") and/or add a concise
inline comment above the normalizeSignatureID function explaining that "0" is
treated as an unset/placeholder ID and should be treated the same as an empty
string, then use that constant in normalizeSignatureID (and update any related
default-selection code/tests to reference the constant) so the convention is
explicit and discoverable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c21a0c96-6a65-4206-a421-a9410006e7fc

📥 Commits

Reviewing files that changed from the base of the PR and between 0d84751 and 8003378.

📒 Files selected for processing (8)
  • shortcuts/mail/draft/htmltext.go
  • shortcuts/mail/mail_send.go
  • shortcuts/mail/mail_send_signature_test.go
  • shortcuts/mail/signature_compose.go
  • shortcuts/mail/signature_compose_test.go
  • skill-template/domains/mail.md
  • skills/lark-mail/SKILL.md
  • skills/lark-mail/references/lark-mail-send.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/mail PR touches the mail domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant