fix: address 2 bugs from Devin Review on workspaceStore#7
Conversation
1. duplicateFile for folders now deep-clones children with new IDs and updated paths instead of copying by reference 2. updateChildrenPaths uses startsWith+slice instead of String.replace to avoid special replacement pattern issues and ensure prefix anchoring Co-Authored-By: Thuong Huynh <admin@huynhthuong.online>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
📝 Info: Pre-existing: dupName regex doesn't handle multi-dot filenames ideally
The dupName construction at line 502 uses /(\.\w+)$/ to split name from extension. For files like App.d.ts, this only captures the last extension segment (.ts), producing App.d (copy).ts instead of App (copy).d.ts. Similarly, for dotfiles like .gitignore, it strips the entire name to produce (copy).gitignore (note the leading space). This is a pre-existing issue not introduced by this PR, but worth noting since the duplicateFile function was modified.
(Refers to line 502)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const updatedChild: FileNode = { | ||
| ...child, | ||
| path: child.path.replace(oldPath, newPath), | ||
| path: child.path.startsWith(oldPath) ? newPath + child.path.slice(oldPath.length) : child.path, |
There was a problem hiding this comment.
📝 Info: The updateChildrenPaths fix correctly addresses a real String.replace bug
The old code child.path.replace(oldPath, newPath) had two issues: (1) String.replace with a string pattern only replaces the first occurrence, which is fine for prefix replacement but semantically unclear; (2) more importantly, it could match oldPath as a substring at any position within child.path, not just at the start. For example, renaming a folder /a to /ab could incorrectly transform a child path /b/a/file to /b/ab/file if the old path appeared as a substring elsewhere. The new startsWith check is strictly correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Fixes 2 bugs identified by Devin Review on PR #3:
duplicateFilefor folders creates children with duplicate IDs and stale paths — When duplicating a folder,newNodewas constructed via...nodespread, which copied thechildrenarray by reference. Every child inside a duplicated folder retained its originalidand originalpath(pointing to the old parent). AddeddeepCloneFileNode()helper that recursively clones a FileNode tree, generating new IDs viagenIdand updating paths for each child.updateChildrenPathsusesString.replacewhich isn't anchored to the path prefix —child.path.replace(oldPath, newPath)treats$&,$`,$', and$$as special replacement patterns innewPath, which could corrupt paths if a folder is renamed to a name containing those characters. Replaced withstartsWith+slicefor safe, prefix-anchored path updates.Review & Testing Checklist for Human
/src/styles→ Duplicate) and verify the copy's children have unique IDs and paths pointing to the new parent folder (e.g./src/styles (copy)/global.css)$&or$$and verify children's paths are updated correctly without corruptionNotes
src/stores/workspaceStore.tswas modifiedLink to Devin session: https://app.devin.ai/sessions/18093fc66df54f558a2031925bd13886
Requested by: @Huynhthuongg