Post move selection by cdvv7788 · Pull Request #12660 · nextcloud/mail

cdvv7788 · 2026-03-25T21:00:18Z

After moving a message (via action menu, thread view, or drag-and-drop), the app now auto-selects the adjacent message in the list instead of leaving the right pane empty or jumping to the topmost message.

Changes

Core logic (Mailbox.vue)

Extracted navigateToAdjacentEnvelope() from the existing onDelete method — picks the nearest envelope above (or
below if at the top), skipping any that are also being moved
Added onMove() handler for move events, reusing the same navigation logic
Added onEnvelopesDropped() listener on dragEventBus for drag-and-drop moves

Event wiring

MoveModal.vue — emits move with envelope IDs before the move happens (so the envelope is still in the list for
index lookup)
Envelope.vue — forwards move events with databaseId
EnvelopeList.vue — forwards @Move to parent, same pattern as @delete
ThreadEnvelope.vue — passes through move event with ID
Thread.vue — emits move instead of manually routing to mailbox root
MailboxThread.vue — bridges @Move from Thread to the bus
NavigationMailbox.vue — removed redundant onEnvelopesMoved handler

Tests

Added Mailbox.vue.spec.js with 15 tests covering navigation, delete, move, bulk move, and drag-and-drop scenarios
Fixed locale-dependent test in groupedEnvelopes.spec.js (hardcoded "July" vs system locale)

I am not sure about how to test the Threads in the UI. I manually tested the drag n drop, the menu at the right of each envelope, and the bulk move.

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

src/tests/unit/components/Mailbox.vue.spec.js

Copilot

Pull request overview

This PR fixes post-move behavior in the Mail app by ensuring that when the currently opened message is moved (via action menu, thread view, or drag-and-drop), the UI automatically navigates to an adjacent message instead of leaving the detail pane empty or jumping to the top.

Changes:

Added shared “select adjacent envelope” navigation logic in Mailbox.vue, reused for delete, move, and drag-and-drop move scenarios.
Rewired move events across list, thread, and modal components so navigation can happen before the moved envelope disappears from the list.
Added a new unit test suite for Mailbox.vue behavior and fixed a locale-dependent test in grouped-envelope date grouping.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/components/Mailbox.vue	Adds move/drag-drop handlers and shared adjacent-selection navigation logic.
src/components/MoveModal.vue	Emits moved envelope IDs before executing the move to enable correct list-index navigation.
src/components/Envelope.vue	Forwards move events with envelope IDs; emits move for quick-move actions.
src/components/EnvelopeList.vue	Forwards `@move` events upward (mirrors existing delete forwarding pattern).
src/components/ThreadEnvelope.vue	Forwards move events with envelope IDs.
src/components/Thread.vue	Emits `move` upward for Mailbox-level navigation instead of routing to mailbox root.
src/components/MailboxThread.vue	Bridges thread `move` events onto the shared bus.
src/components/NavigationMailbox.vue	Removes drag-and-drop post-move navigation to mailbox root.
src/tests/unit/components/Mailbox.vue.spec.js	Adds unit tests for adjacent-navigation behavior on delete/move/drag-drop.
src/tests/unit/util/groupedEnvelopes.spec.js	Fixes locale-dependent assertion for month label grouping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T23:47:54Z

src/components/NavigationMailbox.vue

 	mounted() {
 		dragEventBus.on('drag-start', this.onDragStart)
 		dragEventBus.on('drag-end', this.onDragEnd)
-		dragEventBus.on('envelopes-moved', this.onEnvelopesMoved)
 	},


With the envelopes-moved handler removed here, dragEventBus.emit('envelopes-moved', ...) (still emitted by the droppable-mailbox directive) currently has no listeners in the app. Either remove that emit as dead code or re-home any remaining side effects that still need to run after the move finishes.

Copilot · 2026-03-25T23:47:54Z

src/components/Mailbox.vue

 			const idx = findIndex(propEq(id, 'databaseId'), this.envelopes)
 			if (idx === -1) {
-				logger.debug('envelope to delete does not exist in envelope list')
+				logger.debug('envelope does not exist in envelope list')
 				return


navigateToAdjacentEnvelope() uses findIndex(propEq(id, 'databaseId'), this.envelopes). propEq is strict-equality, so this will fail if route/thread IDs are numbers but envelope.databaseId values (or the passed id) are strings (or vice versa), preventing any navigation after delete/move in real routes (Thread.vue parses $route.params.threadId as int). Normalize IDs before comparing (e.g., compare String(databaseId) values, or parse both to numbers consistently).

same issue, read the other comments below

Copilot · 2026-03-25T23:47:55Z

src/components/Mailbox.vue

 			if (id !== this.$route.params.threadId) {
 				logger.debug('other message open, not jumping to the next/previous message')
 				return


This check is type-sensitive: id is often a number (env.databaseId), while this.$route.params.threadId is typically a string from the URL. That makes the guard incorrectly treat the current message as “different”, so it won’t navigate after delete/move. Convert both sides to the same type before comparing.

Read the comment below. Same issue.

Copilot · 2026-03-25T23:47:55Z

src/components/Mailbox.vue

+			// Find the nearest envelope that is not being moved, preferring
+			// the one above (idx - 1) unless we're at the top of the list
+			let next
+			if (idx === 0) {
+				next = this.envelopes.slice(idx + 1).find((env) => !excludeIds.includes(env.databaseId))
+			} else {
+				next = this.envelopes.slice(0, idx).reverse().find((env) => !excludeIds.includes(env.databaseId))
+					|| this.envelopes.slice(idx + 1).find((env) => !excludeIds.includes(env.databaseId))


excludeIds.includes(env.databaseId) is also type-sensitive and becomes O(n*k) for bulk moves. Consider normalizing excludeIds (e.g., String(id)) and using a Set for membership checks so moved envelopes are reliably skipped and the lookup stays O(1).

Suggested change

// Find the nearest envelope that is not being moved, preferring

// the one above (idx - 1) unless we're at the top of the list

let next

if (idx === 0) {

next = this.envelopes.slice(idx + 1).find((env) => !excludeIds.includes(env.databaseId))

} else {

next = this.envelopes.slice(0, idx).reverse().find((env) => !excludeIds.includes(env.databaseId))

|| this.envelopes.slice(idx + 1).find((env) => !excludeIds.includes(env.databaseId))

// Normalize excludeIds to strings and use a Set for O(1), type-robust membership checks

const excludeIdSet = new Set(excludeIds.map(String))

// Find the nearest envelope that is not being moved, preferring

// the one above (idx - 1) unless we're at the top of the list

let next

if (idx === 0) {

next = this.envelopes.slice(idx + 1).find((env) => !excludeIdSet.has(String(env.databaseId)))

} else {

next = this.envelopes.slice(0, idx).reverse().find((env) => !excludeIdSet.has(String(env.databaseId)))

|| this.envelopes.slice(idx + 1).find((env) => !excludeIdSet.has(String(env.databaseId)))

Slightly more complexity for practically very low gains. I don't think moving thousands of emails is a common occurrence. I can do it, but should be ok to keep it as is.

Copilot · 2026-03-25T23:47:55Z

src/components/Mailbox.vue

+			const currentThreadId = this.$route.params.threadId
+			const movedId = ids.find((id) => id === currentThreadId)
+			if (movedId !== undefined) {
+				this.navigateToAdjacentEnvelope(movedId, ids)


onMove() compares ids to this.$route.params.threadId with strict equality, but ids are frequently numbers while the route param is a string. As a result, move navigation won’t trigger in common cases (e.g., thread view emits numeric ids). Normalize types (String/Number) for the membership check and when passing excludeIds to navigateToAdjacentEnvelope().

Suggested change

const currentThreadId = this.$route.params.threadId

const movedId = ids.find((id) => id === currentThreadId)

if (movedId !== undefined) {

this.navigateToAdjacentEnvelope(movedId, ids)

const currentThreadIdRaw = this.$route.params.threadId

if (currentThreadIdRaw === undefined || currentThreadIdRaw === null) {

return

}

const idsAreNumbers = ids.some((id) => typeof id === 'number')

const currentThreadId = idsAreNumbers ? Number(currentThreadIdRaw) : String(currentThreadIdRaw)

const movedId = ids.find((id) => id === currentThreadId)

if (movedId !== undefined) {

const normalizedExcludeIds = idsAreNumbers

? ids.map((id) => Number(id))

: ids.map((id) => String(id))

this.navigateToAdjacentEnvelope(movedId, normalizedExcludeIds)

The delete path already used this approach https://github.com/nextcloud/mail/blob/main/src/components/Mailbox.vue#L556
If this is going to be changed maybe it should be in it's own issue? It works in practice as-is, and I don't want to touch more than what I need to fix the specific issue the PR is about.

Copilot · 2026-03-25T23:47:55Z

src/tests/unit/components/Mailbox.vue.spec.js

+const envelopes = [
+	{ databaseId: 'A', mailboxId: 1 },
+	{ databaseId: 'B', mailboxId: 1 },
+	{ databaseId: 'C', mailboxId: 1 },
+	{ databaseId: 'D', mailboxId: 1 },
+	{ databaseId: 'E', mailboxId: 1 },
+]


These tests use non-numeric databaseId values ('A'..'E') and threadId route params that match by string. In production, databaseId/threadId are typically numeric (e.g., Thread.vue parses $route.params.threadId with parseInt), so the current tests won’t catch the real-world type mismatch that can block navigation. Consider switching the fixtures to numeric IDs and include at least one case where route params are strings but envelope IDs are numbers.

Same as above.

Co-authored-by: Daniel <mail@danielkesselberg.de> Signed-off-by: Cristian Vargas <cristianvargasvalencia@gmail.com>

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

cdvv7788 · 2026-03-26T15:47:09Z

I fixed the one that made the most sense...the rest are questionable.

jancborchardt

Behavior-wise, 👍 as discussed in #7548 – but I didn’t test.

@cdvv7788 if you have a before/after screenshare that’s appreciated for quick design review.

ChristophWurst · 2026-03-30T11:30:14Z

Thank you for your contribution.

Small head-up about the commit history. 083bd5e is a refactoring. This PR is generally a bug fix. After review approval commits should be squashed into a single fix commit.

See also https://github.com/nextcloud/mail/blob/main/.github/CONTRIBUTING.md#commit-messages.

cdvv7788 added 4 commits March 25, 2026 16:51

feat: Emit moves so the right message is selected

083bd5e

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

fix: bulk moves were not selecting a new messages

0a8ca1f

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

test: Add tests for move related functions/components

2dbaee6

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

chore: Fix lint

63679c2

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

cdvv7788 requested review from ChristophWurst, GretaD and kesselb as code owners March 25, 2026 21:00

kesselb reviewed Mar 25, 2026

View reviewed changes

src/tests/unit/components/Mailbox.vue.spec.js Outdated Show resolved Hide resolved

kesselb requested a review from Copilot March 25, 2026 23:42

Copilot started reviewing on behalf of kesselb March 25, 2026 23:42 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

cdvv7788 and others added 2 commits March 26, 2026 00:07

Update src/tests/unit/components/Mailbox.vue.spec.js

3f725a1

Co-authored-by: Daniel <mail@danielkesselberg.de> Signed-off-by: Cristian Vargas <cristianvargasvalencia@gmail.com>

fix: Remove dead code

4517f32

Signed-off-by: Cristian <cristianvargasvalencia@gmail.com>

jancborchardt reviewed Mar 30, 2026

View reviewed changes

ChristophWurst added 3. to review bug labels Mar 30, 2026

-			const currentThreadId = this.$route.params.threadId
-			const movedId = ids.find((id) => id === currentThreadId)
-			if (movedId !== undefined) {
-				this.navigateToAdjacentEnvelope(movedId, ids)
+			const currentThreadIdRaw = this.$route.params.threadId
+			if (currentThreadIdRaw === undefined || currentThreadIdRaw === null) {
+				return
+			}
+			const idsAreNumbers = ids.some((id) => typeof id === 'number')
+			const currentThreadId = idsAreNumbers ? Number(currentThreadIdRaw) : String(currentThreadIdRaw)
+			const movedId = ids.find((id) => id === currentThreadId)
+			if (movedId !== undefined) {
+				const normalizedExcludeIds = idsAreNumbers
+					? ids.map((id) => Number(id))
+					: ids.map((id) => String(id))
+				this.navigateToAdjacentEnvelope(movedId, normalizedExcludeIds)

Conversation

cdvv7788 commented Mar 25, 2026

Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

cdvv7788 commented Mar 26, 2026

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants