feat: append default mail signatures#1342
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 (12)
📝 WalkthroughWalkthroughThis PR adds automatic default-signature support to mail compose shortcuts by introducing a ChangesDefault signature auto-append for mail compose
Sequence DiagramsequenceDiagram
participant User as User CLI
participant Shortcut as Compose Shortcut
participant Validator as validateSignatureFlags
participant Resolver as resolveComposeSignature
participant Default as defaultSignatureIDFromResponse
participant SigAPI as Signature API
participant Converter as signatureToPlainText
participant Body as Body Builder
User->>Shortcut: --from "user@example.com" [--signature-id "sig_123" | --no-signature]
Shortcut->>Validator: validate flags
Validator-->>Shortcut: ✓ or ValidationError
Shortcut->>Resolver: resolve signature (senderEmail, sigKind)
alt --no-signature set
Resolver-->>Shortcut: nil signature
else --signature-id explicit
Resolver->>SigAPI: fetch signature by ID
SigAPI-->>Resolver: signature HTML + images
else auto-select
Resolver->>Default: find default for sender+kind
Default-->>Resolver: signature ID or "0"
Resolver->>SigAPI: fetch signature
SigAPI-->>Resolver: signature HTML + images
end
alt HTML mode
Resolver-->>Shortcut: signatureResult (HTML)
else plaintext mode
Resolver->>Converter: convert HTML to plaintext
Converter-->>Resolver: plaintext signature
Resolver-->>Shortcut: signatureResult (plaintext)
end
Shortcut->>Body: compose body with signature
Body->>Body: appendPlainTextSignature()
Body-->>Shortcut: final message body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR follows a consistent refactoring pattern across five similar shortcuts (add flag, update validation, refactor signature resolution, append plaintext signatures), but introduces new signature resolution infrastructure in Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1342 +/- ##
=======================================
Coverage 71.58% 71.58%
=======================================
Files 689 689
Lines 65521 65578 +57
=======================================
+ Hits 46901 46943 +42
- Misses 14972 14981 +9
- Partials 3648 3654 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fc2422596eafb3805d6408b6f2a018b96bc337a5🧩 Skill updatenpx skills add sysuljx/cli#feat/f81fd50 -y -g |
cnzakii
left a comment
cnzakii
left a comment
There was a problem hiding this comment.
🤖 AI Review | CR 汇总 | 有风险（1 个 P2，2 个 P3）
自动追加默认签名功能整体实现清晰：nil 安全、best-effort 降级（签名查询失败仅告警不阻断发信）、测试覆盖核心转换与匹配逻辑，未发现 P0/P1 问题。
- P2：
defaultSignatureIDFromResponse在发件地址未命中时回退到usages[0]，且+draft-create未用resolveComposeSenderEmail解析发件地址，组合下可能附加其他身份的默认签名。 - P3：纯文本签名降级丢失超链接 URL；每次 compose 新增一次
signature.ListAll调用。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| } | ||
| } | ||
| if len(resp.Usages) > 0 { | ||
| return normalizeSigID(pick(resp.Usages[0])) |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] 默认签名匹配不到发件地址时回退到 usages[0]，可能附加其他身份的签名
defaultSignatureIDFromResponse 在 senderEmail 未命中任何 usage（或为空）时，统一回退到 resp.Usages[0] 的默认签名（本块第 80-82 行）。这条回退路径在 +draft-create 中很容易被触发：mail_draft_create.go:184-187 仅用裸 --from / --mailbox 解析发件地址，未像 +send/+reply/+forward 那样调用 resolveComposeSenderEmail 兜底到 profile 主地址。因此用户不带 --from、--mailbox 默认 me 时，senderEmail 为空 → 命中此回退 → 取 usages[0]。
影响：当账号存在多个发信身份（别名/共享邮箱）且 usages[0] 并非用户实际发件地址时，会把另一身份的默认签名附加到邮件正文，造成身份信息不一致；同样输入下 +send 解析出主地址精确匹配、+draft-create 却落到首条 usage，两者结果可能不同。
修复建议: 让 +draft-create 复用 resolveComposeSenderEmail(runtime) 解析发件地址（与其余 compose sibling 对齐）；并考虑当 senderEmail 非空但未命中任何 usage 时返回空（不附加签名），而非静默回退到 usages[0]。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| plainTextBlankLinesRE = regexp.MustCompile(`\n{3,}`) | ||
| ) | ||
|
|
||
| func signatureToPlainText(renderedHTML string) string { |
There was a problem hiding this comment.
🤖 AI Review | [P3 正确性] 纯文本签名降级丢失超链接 URL
signatureToPlainText 用 plainTextTagRE（<[^>]+>）剥离所有标签，<a href="https://...">官网</a> 会被压成纯文字 官网，URL 丢失；<img> 也整体删除。对于包含网址/社交链接的签名，纯文本邮件（或纯文本降级路径）收到的签名信息量低于客户端展示。
修复建议: 非阻塞。若要与客户端纯文本签名对齐，可在剥离 <a> 前把 href 提取为 文字 (URL) 形式；否则在文档中说明纯文本签名为有损降级即可。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| if signatureID != "" { | ||
| return signatureID, false | ||
| } | ||
| return resolveDefaultSignatureID(runtime, mailboxID, senderEmail, kind), true |
There was a problem hiding this comment.
🤖 AI Review | [P3 性能] 每次 compose 都会新增一次 signature.ListAll 调用
未显式传 --signature-id/--no-signature 时（默认路径），resolveComposeSignatureID 会调用 resolveDefaultSignatureID → signature.ListAll，即每次 +send/+reply/+forward/+draft-create 都多一次签名列表查询；命中默认签名后还会再触发 signature.Get + send_as + 图片下载。对于从不使用签名的用户，也会在每次发信时付出一次列表查询的网络往返。
修复建议: 属功能固有成本，可接受。若关注热路径延迟，可考虑在单次进程内缓存 ListAll 结果，或在文档中提示可用 --no-signature 跳过。确认为预期即可忽略。
如有疑问或认为判断不准确，欢迎直接回复讨论。
2 participants
Generated by the harness-coding skill.
Sprints
Source specs
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
--no-signatureflag to draft, forward, reply, reply-all, and send commands to skip all signaturesDocumentation