nextcloud / nextcloud/mail

Move-folder modal: self-move guard is dead code and leaves moving stuck

Open
#13,534 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

1. to develop bug
Dominant language
JavaScript
Stars
1k
Forks
348
Avg merge
12h 28m
Merged PRs (30d)
91

Description

Summary

Flagged by Claude during a code review.
Needs validation.

Problem

onMove() sets this.moving = true, then guards the whole operation with this.mailbox.id !== this.destMailboxId. Two defects:

  1. The comparison can never be true. Mailbox::jsonSerialize (lib/Db/Mailbox.php:176-177) exposes id as base64_encode($name), while destMailboxId is always the numeric databaseId or undefined. The intended "moving a folder into itself is a no-op" check therefore never fires.
  2. The false branch resets nothing — no moving = false, no $emit('close'). Only the try/finally inside the if does that.
Consequences
  • Selecting root for a folder that already sits at root issues a rename to its own name; the resulting IMAP error is only logged, so the user sees a silent no-op instead of the guard.
  • Correcting the comparison to databaseId without also fixing the state handling would hang the dialog: since #13431 made the handler's promise the one NcDialogButton awaits, the primary button would stay disabled forever.
Suggested fix

Compare this.mailbox.databaseId against this.destMailboxId, and return before setting moving = true when they match (or hoist the finally cleanup to cover the early exit).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Locate the move-folder modal's onMove() handler and inspect lib/Db/Mailbox.php:176-177 to confirm the mailbox identifier formats. Trace the promise and cleanup behavior around moving and NcDialogButton, then verify that self-moves return without starting the operation and that other paths reset state and close normally.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, php
Domain
frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.