Skip to content

Post move selection #12660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cdvv7788 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Post move selection #12660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
083bd5e
feat: Emit moves so the right message is selected
cdvv7788 Mar 25, 2026
0a8ca1f
fix: bulk moves were not selecting a new messages
cdvv7788 Mar 25, 2026
2dbaee6
test: Add tests for move related functions/components
cdvv7788 Mar 25, 2026
63679c2
chore: Fix lint
cdvv7788 Mar 25, 2026
3f725a1
Update src/tests/unit/components/Mailbox.vue.spec.js
cdvv7788 Mar 26, 2026
4517f32
fix: Remove dead code
cdvv7788 Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/components/Envelope.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,11 +1387,12 @@ export default {
this.showEventModal = true
},

onMove() {
this.$emit('move')
onMove(envelopeIds) {
this.$emit('move', envelopeIds)
},

async moveThread(destMailboxId) {
this.$emit('move', [this.data.databaseId])
if (this.layoutMessageViewThreaded) {
await this.mainStore.moveThread({
envelope: this.data,
Expand All @@ -1403,7 +1404,6 @@ export default {
destMailboxId,
})
}
this.onMove()
},

onCloseMoveModal() {
Expand Down
2 changes: 2 additions & 0 deletions src/components/EnvelopeList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
:account="account"
:envelopes="selectedEnvelopes"
:move-thread="true"
@move="(ids) => $emit('move', ids)"
@close="onCloseMoveModal" />
</div>
</transition>
Expand All @@ -134,6 +135,7 @@
:selected-envelopes="selectedEnvelopes"
:compact-mode="compactMode"
@delete="$emit('delete', env.databaseId)"
@move="(ids) => $emit('move', ids)"
@update:selected="onEnvelopeSelectToggle(env, index, $event)"
@select-multiple="onEnvelopeSelectMultiple(env, index)"
@open:quick-actions-settings="showQuickActionsSettings = true" />
Expand Down
60 changes: 48 additions & 12 deletions src/components/Mailbox.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
:loading-more="false"
:load-more-button="false"
:skip-transition="skipListTransition"
@delete="onDelete" />
@delete="onDelete"
@move="onMove" />
</div>
</template>
<EnvelopeList
Expand All @@ -42,6 +43,7 @@
:load-more-button="showLoadMore"
:skip-transition="skipListTransition"
@delete="onDelete"
@move="onMove"
@load-more="loadMore" />
</div>
</template>
Expand All @@ -62,6 +64,7 @@
import { matchError } from '../errors/match.js'
import NoTrashMailboxConfiguredError
from '../errors/NoTrashMailboxConfiguredError.js'
import dragEventBus from '../directives/drag-and-drop/util/dragEventBus.js'

Check failure on line 67 in src/components/Mailbox.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

Expected "../directives/drag-and-drop/util/dragEventBus.js" to come before "../errors/NoTrashMailboxConfiguredError.js"
import logger from '../logger.js'
import useMainStore from '../store/mainStore.js'
import { mailboxHasRights } from '../util/acl.js'
Expand Down Expand Up @@ -198,8 +201,10 @@
created() {
this.bus.on('load-more', this.onScroll)
this.bus.on('delete', this.onDelete)
this.bus.on('move', this.onMove)
this.bus.on('archive', this.onArchive)
this.bus.on('shortcut', this.handleShortcut)
dragEventBus.on('envelopes-dropped', this.onEnvelopesDropped)
this.loadMailboxInterval = setInterval(this.loadMailbox, 60000)
},

Expand All @@ -220,8 +225,10 @@
unmounted() {
this.bus.off('load-more', this.onScroll)
this.bus.off('delete', this.onDelete)
this.bus.off('move', this.onMove)
this.bus.off('archive', this.onArchive)
this.bus.off('shortcut', this.handleShortcut)
dragEventBus.off('envelopes-dropped', this.onEnvelopesDropped)
this.stopInterval()
},

Expand Down Expand Up @@ -539,26 +546,38 @@
}
},

// onDelete(id): Load more message and navigate to other message if needed
// id: The id of the message being delete
onDelete(id) {
// Get a new message
this.mainStore.fetchNextEnvelopes({
mailboxId: this.mailbox.databaseId,
query: this.searchQuery,
quantity: 1,
})
onEnvelopesDropped({ envelopes }) {
const currentThreadId = this.$route.params.threadId
if (!currentThreadId) {
return
}
const movedIds = envelopes.map((e) => e.databaseId)
const wasMoved = movedIds.some((id) => String(id) === String(currentThreadId))
if (wasMoved) {
this.onMove(movedIds)
}
},

navigateToAdjacentEnvelope(id, excludeIds = []) {
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
Comment on lines 562 to 565
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@cdvv7788 cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same issue, read the other comments below

}
if (id !== this.$route.params.threadId) {
logger.debug('other message open, not jumping to the next/previous message')
return
Comment on lines 567 to 569
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@cdvv7788 cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the comment below. Same issue.

}

const next = this.envelopes[idx === 0 ? 1 : idx - 1]
// 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))
Comment on lines +572 to +579
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@cdvv7788 cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (!next) {
logger.debug('no next/previous envelope, not navigating')
return
Expand All @@ -576,6 +595,23 @@
})
},

onDelete(id) {
this.mainStore.fetchNextEnvelopes({
mailboxId: this.mailbox.databaseId,
query: this.searchQuery,
quantity: 1,
})
this.navigateToAdjacentEnvelope(id, [id])
},

onMove(ids) {
const currentThreadId = this.$route.params.threadId
const movedId = ids.find((id) => id === currentThreadId)
if (movedId !== undefined) {
this.navigateToAdjacentEnvelope(movedId, ids)
Comment on lines +608 to +611
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@cdvv7788 cdvv7788 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
},

onScroll() {
if (this.paginate !== 'scroll') {
logger.debug('ignoring scroll pagination')
Expand Down
6 changes: 5 additions & 1 deletion src/components/MailboxThread.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@
</div>
</template>

<Thread v-if="showThread" :current-account-email="account.emailAddress" @delete="deleteMessage" />
<Thread v-if="showThread" :current-account-email="account.emailAddress" @delete="deleteMessage" @move="moveMessage" />

Check failure on line 193 in src/components/MailboxThread.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

'@Move' should be on a new line
<NoMessageSelected v-else-if="hasEnvelopes" />
</AppContent>
</template>
Expand Down Expand Up @@ -497,6 +497,10 @@
this.bus.emit('delete', id)
},

moveMessage(ids) {
this.bus.emit('move', ids)
},

onScroll(event) {
logger.debug('scroll', { event })

Expand Down
5 changes: 4 additions & 1 deletion src/components/MoveModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ export default {
return
}

// Emit before the move so the parent can navigate to the next message
// while the envelopes are still in the list
this.$emit('move', envelopes.map((envelope) => envelope.databaseId))

for (const envelope of envelopes) {
if (this.moveThread) {
await this.mainStore.moveThread({ envelope, destMailboxId: this.destMailboxId })
Expand All @@ -78,7 +82,6 @@ export default {
}

await this.mainStore.syncEnvelopes({ mailboxId: this.destMailboxId })
this.$emit('move')
} catch (error) {
logger.error('could not move messages', {
error,
Expand Down
22 changes: 0 additions & 22 deletions src/components/NavigationMailbox.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,13 +514,11 @@ export default {
mounted() {
dragEventBus.on('drag-start', this.onDragStart)
dragEventBus.on('drag-end', this.onDragEnd)
dragEventBus.on('envelopes-moved', this.onEnvelopesMoved)
},
Comment on lines 514 to 517
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

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

methods: {
Expand Down Expand Up @@ -763,26 +761,6 @@ export default {
this.showSubMailboxes = false
},

onEnvelopesMoved({ mailboxId, movedEnvelopes }) {
if (this.mailbox.databaseId !== mailboxId) {
return
}
const openedMessageHasBeenMoved = movedEnvelopes.find((movedEnvelope) => {
return movedEnvelope.envelopeId === this.$route.params.threadId
})
// navigate to the mailbox root
// if the currently displayed message has been moved
if (this.$route.name === 'message' && openedMessageHasBeenMoved) {
this.$router.push({
name: 'mailbox',
params: {
mailboxId: this.$route.params.mailboxId,
filter: this.$route.params?.filter,
},
})
}
},

/**
* Delete all vanished emails that are still cached.
*
Expand Down
8 changes: 2 additions & 6 deletions src/components/Thread.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,8 @@ export default {

onMove(threadId) {
if (threadId === this.threadId) {
this.$router.replace({
name: 'mailbox',
params: {
mailboxId: this.$route.params.mailboxId,
},
})
// Let Mailbox.vue handle navigation to the next message
this.$emit('move', [threadId])
} else {
this.expandedThreads = this.expandedThreads.filter((id) => id !== threadId)
this.fetchThread()
Expand Down
4 changes: 2 additions & 2 deletions src/components/ThreadEnvelope.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,8 +1044,8 @@ export default {
}
},

onMove() {
this.$emit('move')
onMove(envelopeIds) {
this.$emit('move', envelopeIds)
},

onOpenMoveModal() {
Expand Down
5 changes: 0 additions & 5 deletions src/directives/drag-and-drop/droppable-mailbox/droppable-mailbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,6 @@ export class DroppableMailbox {
await Promise.all(processedEnvelopes)
} catch (error) {
logger.error('could not process dropped messages', error)
} finally {
dragEventBus.emit('envelopes-moved', {
mailboxId: this.options.mailboxId,
movedEnvelopes: envelopesBeingDragged,
})
}
}

Expand Down
Loading
Loading