fix: ModifyId，假如新ID和旧ID的NonDX相同（例如5003->15003），会导致源文件被误删除且发生报错。#66
Merged
Conversation
Contributor
Reviewer's Guide通过将 jacket/audio/video（封面/音频/视频）的迁移逻辑抽取到一个辅助方法中，并在当新 ID 的 NonDx 部分未变化时提前返回，避免在像 5003→15003 这类共享相同 NonDx 的 ID 变更中意外删除源资源；同时在该路径下将 DeleteIfExists 的作用范围限制为仅非谱面（chart 以外）的资源，并将谱面文件夹清理逻辑单独处理。 更新后的 ModifyId 资源移动逻辑顺序图sequenceDiagram
participant Controller as MusicTransferController
participant Settings as settings
participant Music as MusicXmlWithABJacket
Controller->>Settings: GetMusic(id, assetDir)
Settings-->>Controller: Music
alt music is null or license inactive
Controller-->>Controller: return
else valid music
Controller->>Controller: DeleteIfExists(newMusicDir)
Controller->>Controller: MoveJacketSoundVideo(music, newId, assetDir)
activate Controller
Controller->>Controller: [compute newNonDxId]
alt newNonDxId == music.NonDxId
Controller-->>Controller: return (skip jacket/audio/video move)
else newNonDxId != music.NonDxId
Controller->>Controller: DeleteIfExists(jacketTarget, soundTarget, movieTarget)
Controller->>Controller: move jacket/audio/video files
end
deactivate Controller
Controller->>Controller: move chart files (music directory)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来：
Getting HelpOriginal review guide in EnglishReviewer's GuideRefactors ModifyId by extracting jacket/audio/video relocation into a helper that early-returns when the new ID’s NonDx portion is unchanged, preventing accidental deletion of source assets when IDs like 5003→15003 share the same NonDx, and limits DeleteIfExists in that path to only non-chart assets while handling chart folder cleanup separately. Sequence diagram for updated ModifyId asset move logicsequenceDiagram
participant Controller as MusicTransferController
participant Settings as settings
participant Music as MusicXmlWithABJacket
Controller->>Settings: GetMusic(id, assetDir)
Settings-->>Controller: Music
alt music is null or license inactive
Controller-->>Controller: return
else valid music
Controller->>Controller: DeleteIfExists(newMusicDir)
Controller->>Controller: MoveJacketSoundVideo(music, newId, assetDir)
activate Controller
Controller->>Controller: [compute newNonDxId]
alt newNonDxId == music.NonDxId
Controller-->>Controller: return (skip jacket/audio/video move)
else newNonDxId != music.NonDxId
Controller->>Controller: DeleteIfExists(jacketTarget, soundTarget, movieTarget)
Controller->>Controller: move jacket/audio/video files
end
deactivate Controller
Controller->>Controller: move chart files (music directory)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ModifyId method in MusicTransferController.cs by extracting the asset-moving logic (jacket, sound, and video) into a helper method MoveJacketSoundVideo. It also skips moving these assets if the non-DX portion of the ID remains unchanged. A critical issue was identified where modifying an ID to the same value (id == newId) would delete the existing music directory, leading to data loss. A suggestion was made to return early in this case.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我在这里给出了一些总体反馈：
MoveJacketSoundVideo中的行内注释写的是5003 -> 105003，而 PR 描述中写的是5003 -> 15003；建议修改这个示例，以避免让人混淆到底是哪种模式真正有问题。- 现在关于夹克/音频/视频的处理已经被抽取到了
MoveJacketSoundVideo中，你可以考虑把这部分逻辑移到一个专门的 service/helper 类中，而不是继续放在 controller 里，这样可以让 controller 的 action 更精简，更专注于 HTTP 相关的职责。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- The inline comment in `MoveJacketSoundVideo` mentions `5003 -> 105003` while the PR description uses `5003 -> 15003`; consider correcting this example to avoid confusion about which pattern is actually problematic.
- Now that the jacket/audio/video handling is extracted to `MoveJacketSoundVideo`, you might consider moving this logic into a dedicated service/helper class instead of keeping it on the controller to keep the controller action thin and focused on HTTP concerns.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈来改进之后的 Review。
Original comment in English
Hey - I've left some high level feedback:
- The inline comment in
MoveJacketSoundVideomentions5003 -> 105003while the PR description uses5003 -> 15003; consider correcting this example to avoid confusion about which pattern is actually problematic. - Now that the jacket/audio/video handling is extracted to
MoveJacketSoundVideo, you might consider moving this logic into a dedicated service/helper class instead of keeping it on the controller to keep the controller action thin and focused on HTTP concerns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline comment in `MoveJacketSoundVideo` mentions `5003 -> 105003` while the PR description uses `5003 -> 15003`; consider correcting this example to avoid confusion about which pattern is actually problematic.
- Now that the jacket/audio/video handling is extracted to `MoveJacketSoundVideo`, you might consider moving this logic into a dedicated service/helper class instead of keeping it on the controller to keep the controller action thin and focused on HTTP concerns.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Member
这两种应该是一样的吧，一种是 dx 一种是宴谱 |
Collaborator
Author
逻辑上当然没有差别，现在我改完的代码也两种情况都能处理。 他的意思是我代码注释中举的例子，和commit message中举的例子，文字表述不一致。 |
clansty
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
如题，ModifyId，假如新ID和旧ID的NonDX相同（例如5003->15003），会导致源文件被误删除且发生报错。
之前的逻辑会计算新ID下文件移动到的目标路径，如果已经存在冲突的同名文件文件则删除之，再尝试移动旧ID下的文件过来。
那么当5003->15003时，目标计算出的还是
ui_jacket_005003.jpg，于是这个“如果已经存在冲突的同名文件文件则删除之”的过程直接把源文件干掉了。根据OPT中文件的排列规则，封面图、音频、视频用的都是nonDX四位ID，只有谱面
music目录下用的六位长ID。所以当上述“新ID和旧ID的NonDX相同”的情况下，封面图、音频、视频根本不需要做任何移动。所以把移动封面图、音频、视频的过程单独抽成一个函数，如果新ID和旧ID的NonDX相同则直接return、跳过移动它们的过程。谱面music则仍是每次正常移动。
Summary by Sourcery
通过将封面 / 音频 / 视频 的处理与谱面数据迁移分离，防止在新 ID 与旧 ID 具有相同非 DX 部分时，
ModifyId误删、误移动媒体文件。Bug Fixes:
Enhancements:
ModifyId调用的专用辅助方法中，从而与谱面文件处理实现更清晰的职责分离。Original summary in English
Summary by Sourcery
Prevent ModifyId from incorrectly deleting and moving media files when the new ID shares the same non-DX portion as the old ID by separating jacket/audio/video handling from chart data migration.
Bug Fixes:
Enhancements: