Do not re-insert signature when opening a draft

[jancborchardt]

When you have a signature enabled and open a draft, that signature gets inserted. This can even result in multiple insertions of the signature if you open the draft multiple times. When opening a draft, the signature should never be inserted, as this is already done at initial composition.

Initial composer (with signature removed)	Before when reopening draft	Bonus: Multiple draft reopening	After when reopening draft

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI

Assisted-by: ClaudeCode:claude-opus-4-8

Summary by CodeRabbit

• Bug Fixes
  • Improved signature handling when editing draft messages. Signatures no longer auto-insert when reopening a draft, reducing signature duplication issues. Manually changing the signature selection will still insert the new signature correctly. Draft editing now has separate behavior from new message composition, where signatures continue to auto-insert as before.

[jancborchardt]

(Rebased, all tests were passing before also.)

[ChristophWurst]

@coderabbitai review and find out if there is existing logic to handle this, because this was fixed before

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look at the PR diff first
gh pr diff 13146 --repo nextcloud/mail

echo "=== PR FILES CHANGED ==="
gh pr view 13146 --repo nextcloud/mail --json files --jq '.files[].path'

Length of output: 3936

---

🏁 Script executed:

#!/bin/bash
# Search for existing draft-opening signature guards
echo "=== Signature insertion logic ==="
rg -n "signature" --type vue --type js --type ts -C3 src/ | grep -i "draft\|isDraft\|is_draft" -A3 -B3

echo "=== isDraft checks around signature ==="
rg -nP "isDraft|is_draft" src/ --type vue --type js --type ts -C5

echo "=== Draft-related signature skip logic ==="
rg -n "signature" src/ --type vue --type js --type ts -l

Length of output: 968

---

🏁 Script executed:

#!/bin/bash
# Find all JS/Vue/TS files related to signatures
fd -e vue -e js -e ts . src/ --exec grep -l -i "signature" {} \;

echo "=== Look for draft checks in composer ==="
rg -rn "draft" src/ --include="*.vue" --include="*.js" --include="*.ts" -l

Length of output: 1785

---

🏁 Script executed:

#!/bin/bash
echo "=== Files changed in PR ==="
gh pr diff 13146 --repo nextcloud/mail --name-only

echo ""
echo "=== Full diff ==="
gh pr diff 13146 --repo nextcloud/mail

Length of output: 3944

---

🏁 Script executed:

#!/bin/bash
echo "=== Relevant Composer.vue sections ==="
# Get the insertSignature method and surrounding context
rg -n "insertSignature\|changeSignature\|isFirstOpen\|isDraft\|onEditorReady" src/components/Composer.vue -A2 -B2

echo ""
echo "=== changeSignature data definition ==="
rg -n "changeSignature" src/components/Composer.vue -A5 -B5

echo ""
echo "=== isFirstOpen usage ==="
rg -n "isFirstOpen" src/components/Composer.vue -A5 -B5

Length of output: 4292

---

🏁 Script executed:

#!/bin/bash
echo "=== Git log for signature/draft related changes ==="
git log --oneline --all -50 -- src/components/Composer.vue

echo ""
echo "=== Previous fixes mentioning draft+signature ==="
git log --oneline --all --grep="signature" --grep="draft" --all-match -20
git log --oneline --all --grep="draft.*signature\|signature.*draft" -20

Length of output: 3673

---

🏁 Script executed:

#!/bin/bash
echo "=== InsertSignatureCommand.js ==="
cat -n src/ckeditor/signature/InsertSignatureCommand.js

echo ""
echo "=== SignaturePlugin.js ==="
cat -n src/ckeditor/signature/SignaturePlugin.js

echo ""
echo "=== Git log for InsertSignatureCommand / SignaturePlugin ==="
git log --oneline -20 -- src/ckeditor/signature/InsertSignatureCommand.js
git log --oneline -20 -- src/ckeditor/signature/SignaturePlugin.js

echo ""
echo "=== Was there ever a guard in onEditorReady? Check git log for that area ==="
git log --all --oneline --follow -30 -- src/components/Composer.vue | head -30

Length of output: 10792

---

🏁 Script executed:

#!/bin/bash
# Check if isFirstOpen was previously used to guard insertSignature
# Look at older versions of onEditorReady in Composer.vue
git log --all --oneline -30 -- src/components/Composer.vue

echo ""
# Show any commit that mentions 'signature' in its message related to Composer
git log --all --oneline --grep="signature" -- src/components/Composer.vue

echo ""
# Check if there are any related issues or PRs mentioned in the git log  
git log --all --format="%H %s %b" --grep="draft" --grep="signature" --all-match | head -50

Length of output: 3163

---

---

@ChristophWurst — yes, there is existing logic that was supposed to handle this. Here's what I found:

Existing logic in InsertSignatureCommand.js (Line 145)

if (trigger === TRIGGER_EDITOR_READY && !this.hasSignatureElement(this.editor)) {
    this.insertSignatureElement(this.editor, writer, signature, signatureAboveQuote)
}

This guard only inserts the signature on editor-ready if no <signature> element already exists in the document model. Even the code comment on line 137 explicitly lists the use case as:

"TRIGGER_EDITOR_READY: Insert signature if non exist. Use case: Write new message, Modify signature, Save draft, Close Editor, Open editor"

Why it's insufficient

The existing guard works only when the draft was saved with a signature already in it. If the user deleted the signature before saving the draft, hasSignatureElement() returns false (no <signature> element in the model), so the guard passes and the signature gets re-inserted. That's the regression.

What this PR does differently

The PR adds an isDraft check at the Composer.vue level — before insertSignature() is even called — which prevents signature insertion for all draft-open scenarios regardless of whether the draft body happens to contain a signature element:

if (!this.isDraft || this.changeSignature) {
    this.insertSignature()
}

This is a sound and necessary fix that complements rather than replaces the existing hasSignatureElement() guard (which still serves to prevent duplicate signatures in other scenarios like the TRIGGER_EDITOR_READY path for non-draft new messages).

The approach looks correct. The three new unit tests cover the three relevant cases (new message, draft without alias change, draft with alias change).

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e797e998-c3c6-4904-9169-54c43159137b

📥 Commits

Reviewing files that changed from the base of the PR and between 1ea2f34 and 9e36af9.

📒 Files selected for processing (3)

• src/components/Composer.vue
• src/components/NewMessageModal.vue
• src/tests/unit/components/Composer.vue.spec.js

---

📝 Walkthrough

Walkthrough

A new isDraft boolean prop (default false) is added to Composer.vue. The onEditorReady handler now calls insertSignature() only when the composer is not a draft or when the alias was changed. NewMessageModal.vue passes :is-draft based on the presence of composerData.draftId. Three unit tests cover the new conditional behavior.

Draft-aware signature insertion

Layer / File(s)	Summary
isDraft prop and conditional signature logic src/components/Composer.vue, src/components/NewMessageModal.vue	isDraft: Boolean prop (default false) is added to Composer. onEditorReady replaces the unconditional insertSignature() call with !isDraft || changeSignature guard. NewMessageModal binds :is-draft="composerData.draftId !== undefined" to wire draft detection.
Unit tests for signature insertion conditions src/tests/unit/components/Composer.vue.spec.js	Three new tests assert: insertSignature is called for a new message, not called when isDraft is true, and called when isDraft is true but changeSignature is also true.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐇 A draft is a draft, not a brand new song,
So skip the signature when a draft comes along.
But change your alias? Oh, we'll sign it right!
New messages bloom with a signature bright.
Hop hop, the tests confirm the rule is tight! ✉️

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: preventing signatures from being re-inserted when opening a draft, which is the core bug fix addressed in the PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/draft-signature

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ChristophWurst]

/backport to stable5.10

[kesselb]

I'm seeing in CKEditor inspector, that detecting the signature is broken. Hence the existing logic, to replace the signature block, does not work anymore.

We should fix the detection of the signature.
Not workaround the actual problem.

[ChristophWurst]

Thanks. I knew we had something like that. Rabbit just didn't find it.