Move-folder modal: self-move guard is dead code and leaves moving stuck
Nobody has claimed this yet.
- 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:
- 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.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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