feat(mail): apply default send signatures#1312
Open
oOvalm wants to merge 5 commits into
Open
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds explicit signature control to ChangesMail +send Signature Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/mail_send_signature.go`:
- Around line 189-201: The current appendMailSendSignatureText function always
inserts a separator between the existing buffer and the next text node because
it uses strings.Fields (which discards source leading/trailing whitespace) and
then unconditionally inserts a space when buf isn't empty; instead, preserve
source whitespace semantics: get s := stdhtml.UnescapeString(raw), return if
strings.TrimSpace(s)==""; collapse internal runs of whitespace to a single space
(but do not drop leading/trailing whitespace), then determine whether to write
an extra separator by checking the buffer's last byte for unicode.IsSpace and
the unescaped s's first rune for unicode.IsSpace — only write a space if buf is
non-empty AND neither the buffer ends with whitespace nor s begins with
whitespace — finally write the collapsed-but-trimmed-for-internal-whitespace
text to buf; update appendMailSendSignatureText to use these checks (refer to
appendMailSendSignatureText, stdhtml.UnescapeString, and buf.Bytes()) to fix
inline-markup accidental spacing.
In `@shortcuts/mail/mail_send.go`:
- Around line 209-212: The call to resolveMailSendComposeSignature is passing an
empty senderEmail when --from is omitted, causing
selectMailSendDefaultSignatureID to misapply defaults for mailboxes with
aliases; fix by resolving the actual sender address from the mailbox/profile
before calling resolveMailSendComposeSignature (or update
resolveMailSendComposeSignature to perform that lookup internally) so that
senderEmail is never empty — locate the call site using
resolveMailSendComposeSignature(ctx, runtime, mailboxID, senderEmail,
mailSendSignatureOptions{...}) and ensure you populate senderEmail from the
mailbox/profile primary address (or invoke a helper that does this) prior to
selecting the default via selectMailSendDefaultSignatureID.
🪄 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: ad2c5d69-1138-4475-960a-9994a1e27d2e
📒 Files selected for processing (10)
shortcuts/mail/mail_lint_writepath_test.goshortcuts/mail/mail_request_receipt_integration_test.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_confirm_output_test.goshortcuts/mail/mail_send_signature.goshortcuts/mail/mail_send_signature_test.goshortcuts/mail/mail_template_shortcut_test.goskills/lark-mail/references/lark-mail-send.mdtests/cli_e2e/mail/mail_send_dryrun_test.gotests/cli_e2e/mail/mail_send_workflow_test.go
Author
|
recheck |
Author
|
/recheck |
1 similar comment
Author
|
/recheck |
Author
|
recheck |
2 similar comments
Author
|
recheck |
Author
|
recheck |
Decode escaped HTML fragments inside rendered mail signatures before converting them to text so text/plain sends keep visible text and localized image placeholders instead of tag literals. sprint: S1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Avoid treating ordinary text that starts with a tag-name prefix as escaped signature HTML while preserving the second-pass parsing needed for real HTML fragments. sprint: S1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mail_send_signature.go`:
- Around line 189-198: The code double-unescapes entities by calling
stdhtml.UnescapeString in appendMailSendSignatureTextNode and then again inside
appendMailSendSignatureText; change appendMailSendSignatureTextNode to only use
the unescaped string for the mailSendSignatureLooksLikeHTMLFragment check but
pass the original raw string to appendMailSendSignatureText (both in the
non-HTML branch and the error branch) so appendMailSendSignatureText is solely
responsible for unescaping; refer to appendMailSendSignatureTextNode,
mailSendSignatureLooksLikeHTMLFragment, and appendMailSendSignatureText when
making the change.
🪄 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: 01d41ad4-35fc-42ff-bc5e-e561f45e6b56
📒 Files selected for processing (2)
shortcuts/mail/mail_send_signature.goshortcuts/mail/mail_send_signature_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_send_signature_test.go
When the owning mailbox signature usage list omits a send_as alias, probe the alias mailbox before concluding there is no default signature. The selector still requires EmailAddress to match the final sender, and explicit/no-signature behavior is unchanged. sprint: S1
Author
|
recheck |
Author
|
/recheck |
1 similar comment
Author
|
/recheck |
appendMailSendSignatureTextNode and appendMailSendSignatureText both called stdhtml.UnescapeString on text that xhtml.Parse had already unescaped, corrupting double-encoded entities (e.g. &amp;). Change-Type: ci-fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Documentation
Tests