Skip to content

feat: append default mail send signatures#1341

Open
bubbmon233 wants to merge 3 commits into
larksuite:mainfrom
bubbmon233:feat/eb56b12
Open

feat: append default mail send signatures#1341
bubbmon233 wants to merge 3 commits into
larksuite:mainfrom
bubbmon233:feat/eb56b12

Conversation

@bubbmon233

@bubbmon233 bubbmon233 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Generated by the harness-coding skill.

  • Branch: feat/eb56b12
  • Target: main

Sprints

ID Title Status Commit
S1 Add default signature auto-append to mail +send passed 4a2436a
S2 Synthesize transport contract for larksuite/cli passed 03ea6e7

Source specs

  • 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

    • Added --no-signature to skip automatic signature appending when sending mail.
    • Sender default signature is auto-appended on send; plain-text mode appends a plain-text signature.
  • Improvements

    • Signature lookup failures downgrade to a warning (do not block send).
    • Enforced mutual exclusivity between --no-signature and --signature-id.
  • Documentation

    • Updated mail send/signature docs with flag details and default behavior.
  • Tests

    • Added tests for default signature selection, plain-text appending, lookup warnings, and flag validation.

Add send-only default signature lookup with --no-signature opt-out and plain-text signature rendering.

sprint: S1
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 248e8662-eb0f-4d45-aac5-febc22f9bfcf

📥 Commits

Reviewing files that changed from the base of the PR and between 7d432d6 and 77d4383.

📒 Files selected for processing (1)
  • shortcuts/mail/signature_compose.go

📝 Walkthrough

Walkthrough

This PR adds automatic default send signature selection to mail send with a --no-signature flag to skip it, refactors signature resolution to control inline image inclusion, implements HTML→plain-text signature conversion and appending, updates MailSend wiring/validation/execute logic, and adds tests and docs.

Changes

Default Send Signature with Plain-Text Support

Layer / File(s) Summary
Signature refactor, imports, and flag
shortcuts/mail/signature_compose.go, shortcuts/mail/mail_send.go
Adds HTML/entity imports, introduces noSignatureFlag, and refactors resolveSignature/resolveSignatureTextOnly to delegate to resolveSignatureWithImages with includeImages control.
Default send signature auto-detection
shortcuts/mail/signature_compose.go
Adds resolveDefaultSendSignatureID and defaultSendSignatureIDFromUsages to list signatures and pick a non-empty/non-"0" default SendMailSignatureID by matching sender usages.
Plain-text signature conversion and appending
shortcuts/mail/signature_compose.go, shortcuts/mail/mail_send.go
Implements appendPlainTextSignature and signatureHTMLToPlainText (HTML parse + fallback unescape), normalizes whitespace, and appends plain-text signatures into composed text bodies.
MailSend flags, DryRun, and Validate
shortcuts/mail/mail_send.go
Adds noSignatureFlag to MailSend.Flags, conditionally fetches signatures in DryRun only when not suppressed, and enforces mutual exclusion between --no-signature and --signature-id in Validate.
MailSend execute signature handling
shortcuts/mail/mail_send.go
Clears signature ID when --no-signature is set; otherwise attempts default signature resolution (warns on failure), resolves HTML or text-only signature per --plain-text, and appends plain-text signatures when composing text bodies.
Comprehensive signature behavior test suite
shortcuts/mail/mail_send_signature_test.go
New unit/integration tests covering default signature selection (including "0" semantics), plain-text appending, EML inclusion/exclusion, lookup-failure downgrade warning, --no-signature suppression, plain-text content-type preservation, validation rules, non-send shortcut checks, and HTTP/test helpers.
User-facing documentation updates
skills/lark-mail/references/lark-mail-send.md, skills/lark-mail/references/lark-mail-signature.md
Documents automatic default signature appending, --no-signature option and mutual exclusion with --signature-id, lookup-fallback behavior, plain-text conversion rules, and adds examples.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • larksuite/cli#595: Modifies shortcuts/mail/mail_send.go Validate/Execute logic in related +send code paths.

Suggested reviewers

  • chanthuang
  • infeng

Poem

🐰 I nibble bytes and stitch the sign,
Default picks from sender line,
Plain-text trims the HTML song,
--no-signature hops along,
A tiny hop — the mail goes strong.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is largely incomplete. It lacks required sections including a summary of changes, detailed changelog, test plan verification, and related issues. It only provides metadata about the branch and automated sprint information. Add a Summary section describing the motivation, a Changes section listing main modifications, a Test Plan section documenting verification approach, and a Related Issues section.
Docstring Coverage ⚠️ Warning Docstring coverage is 11.54% 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 title accurately summarizes the main feature: appending default signatures to mail send operations, which is the primary change across the modified files.
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

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 9, 2026
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@77d4383742080bbfe4e72d38b1a5147a0e5470a1

🧩 Skill update

npx skills add bubbmon233/cli#feat/eb56b12 -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.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@shortcuts/mail/signature_compose.go`:
- Around line 176-220: In appendHTMLNodeText, skip non-rendered elements so
their contents are not included in the plain-text output: detect element tags
like "style", "script", "noscript", and "template" (inside the
nethtml.ElementNode switch in appendHTMLNodeText) and simply return without
descending into children; this prevents the recursive fallback from walking into
those nodes and emitting their literal text.
- Around line 98-107: resolveDefaultSendSignatureID currently only considers the
caller-provided senderEmail and mailboxID, so when senderEmail is empty it never
falls back to the primary send-as address; update resolveDefaultSendSignatureID
to first resolve the primary sender (call resolveSenderInfo or equivalent to
obtain the sender email when senderEmail == "") before building candidates, then
pass that resolved email (plus mailboxID handling as before) into
defaultSendSignatureIDFromUsages; keep the existing signature.ListAll call and
error handling and only change how candidates is populated.
🪄 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: 7a111deb-0902-43ac-8476-d55e92318dd4

📥 Commits

Reviewing files that changed from the base of the PR and between 03ea6e7 and 4a2436a.

📒 Files selected for processing (5)
  • shortcuts/mail/mail_send.go
  • shortcuts/mail/mail_send_signature_test.go
  • shortcuts/mail/signature_compose.go
  • skills/lark-mail/references/lark-mail-send.md
  • skills/lark-mail/references/lark-mail-signature.md

Comment thread shortcuts/mail/signature_compose.go
Comment thread shortcuts/mail/signature_compose.go
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.71166% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.63%. Comparing base (03ea6e7) to head (77d4383).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/mail/signature_compose.go 66.66% 35 Missing and 11 partials ⚠️
shortcuts/mail/mail_send.go 80.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1341      +/-   ##
==========================================
+ Coverage   71.58%   71.63%   +0.05%     
==========================================
  Files         689      689              
  Lines       65521    65666     +145     
==========================================
+ Hits        46901    47042     +141     
+ Misses      14972    14969       -3     
- Partials     3648     3655       +7     

☔ 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.

…on-rendered nodes

Address CodeRabbit review on PR larksuite#1341:
- resolveDefaultSendSignatureID now falls back to the primary send-as
  address (via resolveSenderInfo) when senderEmail is empty, so the
  default signature is still matched.
- appendHTMLNodeText skips script/style/head/template/noscript so their
  contents no longer leak into --plain-text signature output.

Change-Type: ci-fix
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.

2 participants