feature: copy and paste recursively in project explorer #8036
Conversation
…writing unit tests for folders only
…sted more and cleaned up
…e filename with empty string to get the last bit
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
src/components/Explorer/utils.ts
Outdated
| export const getUniqueCopyPath = ( | ||
| possibleCollisions: string[], | ||
| possibleCopyPath: string, | ||
| identifer: string, |
There was a problem hiding this comment.
The parameter name identifer appears to be misspelled. It should be identifier to match how it's used throughout the function body and to maintain consistent spelling across the codebase.
| identifer: string, | |
| identifier: string, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
thanks, i can't spell!
nadr0
changed the title
nadr0
changed the title
| <ContextMenuItem data-testid="context-menu-delete" onClick={onCopy}> | ||
| Copy | ||
| </ContextMenuItem>, |
There was a problem hiding this comment.
The data-testid attribute for the Copy menu item is incorrectly set to "context-menu-delete". This will cause confusion during testing and make the UI harder to test reliably. Please update it to "context-menu-copy" to maintain consistent and descriptive test identifiers.
| <ContextMenuItem data-testid="context-menu-delete" onClick={onCopy}> | |
| Copy | |
| </ContextMenuItem>, | |
| <ContextMenuItem data-testid="context-menu-copy" onClick={onCopy}> | |
| Copy | |
| </ContextMenuItem>, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
I think bot is right here, but not going to commit it in case your tests rely on it.
| const absoluteParentPath = getParentAbsolutePath(row.path) | ||
| const parentIndex = flattenedData.findIndex((entry) => { | ||
| return entry.path === absoluteParentPath |
There was a problem hiding this comment.
The parent path calculation appears incorrect for paste operations. Currently, getParentAbsolutePath(row.path) is called on the target row, but this logic doesn't account for the difference between pasting into a folder versus pasting next to a file.
When right-clicking on a file, the paste should occur in that file's parent directory, but when right-clicking on a folder, the paste should occur inside that folder. The current implementation treats both cases the same way.
Consider refactoring this to check if the target is a folder:
const targetPath = row.children ? row.path : getParentAbsolutePath(row.path);
const parentIndex = flattenedData.findIndex((entry) => {
return entry.path === targetPath;
});This would ensure files are pasted in the expected location based on the context of the right-click.
| const absoluteParentPath = getParentAbsolutePath(row.path) | |
| const parentIndex = flattenedData.findIndex((entry) => { | |
| return entry.path === absoluteParentPath | |
| const targetPath = row.children ? row.path : getParentAbsolutePath(row.path) | |
| const parentIndex = flattenedData.findIndex((entry) => { | |
| return entry.path === targetPath |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| const takenNumbers = matches | ||
| .map((matchedPath) => { | ||
| const folderOrFileName = getStringAfterLastSeparator(matchedPath) | ||
| matchedPath = matchedPath.replace(fileName, '') |
There was a problem hiding this comment.
The string replacement operation matchedPath = matchedPath.replace(fileName, '') could lead to incorrect collision detection if fileName appears multiple times in the path. Since replace() only replaces the first occurrence by default, this might work in most cases, but for paths where the filename pattern repeats in parent directories, it could remove the wrong part of the string.
Consider using a more targeted approach that ensures only the filename portion is replaced, perhaps by working with the path components directly or using a regex with end-of-string anchor. This would make the collision detection more robust when handling complex directory structures.
// Example of a more targeted approach
const fileNamePart = new RegExp(`${fileName}$`);
matchedPath = matchedPath.replace(fileNamePart, '');| matchedPath = matchedPath.replace(fileName, '') | |
| matchedPath = matchedPath.replace(new RegExp(`${fileName}$`), '') |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| onError: { | ||
| target: SystemIOMachineStates.idle, | ||
| actions: [SystemIOMachineActions.toastError], | ||
| }}}, |
There was a problem hiding this comment.
There appears to be a syntax error in the state machine definition. The onError block is missing a closing comma after its closing brace, which will cause a JavaScript syntax error when the application tries to run. The correct syntax should be:
onError: {
target: SystemIOMachineStates.idle,
actions: [SystemIOMachineActions.toastError],
},This comma is needed because another state definition follows this block.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
nadr0
force-pushed
the
nadro/gh-7952/cut-copy-paste-file-tree
branch
from
3f4e6ba to
b0df204
Compare
| </ContextMenuItem>, | ||
| <ContextMenuItem | ||
| disabled={!isCopying} | ||
| data-testid="context-menu-delete" |
There was a problem hiding this comment.
Same as the above, should be context-menu-paste.
| const lastIndexBeforeNothing = useRef<number>(-2) | ||
|
|
||
| // Store a path to copy and paste! Works for folders and files | ||
| const copyToClipBoard = useRef<FileEntry | null>(null) |
There was a problem hiding this comment.
I'm curious as to why useRef and not useState here?
There was a problem hiding this comment.
Oh so recursive rows can just mutate directly without having to pass handlers and state all the way down?
And clipboard state is not displayed anywhere in the UI, so fine that it's outside of react lifecycle?
There was a problem hiding this comment.
Correct, the handles can directly access and edit a state value. The current copy path is not rendered in react.
| }, | ||
| }) | ||
| } else { | ||
| console.error('failed to copy and paste the result is null') |
| <ContextMenuItem data-testid="context-menu-delete" onClick={onCopy}> | ||
| Copy | ||
| </ContextMenuItem>, | ||
| <ContextMenuItem | ||
| disabled={!isCopying} | ||
| data-testid="context-menu-delete" | ||
| onClick={onPaste} | ||
| > | ||
| Paste | ||
| </ContextMenuItem>, |
There was a problem hiding this comment.
The data-testid attribute for the "Copy" and "Paste" context menu items is currently set to "context-menu-delete", which is the same identifier used by the Delete item. This will cause ambiguity in automated tests that target these elements by their test IDs.
For better test reliability and clarity, please update these attributes to more specific values such as context-menu-copy and context-menu-paste respectively:
<ContextMenuItem data-testid="context-menu-copy" onClick={onCopy}>
Copy
</ContextMenuItem>,
<ContextMenuItem
disabled={!isCopying}
data-testid="context-menu-paste"
onClick={onPaste}
>
Paste
</ContextMenuItem>,| <ContextMenuItem data-testid="context-menu-delete" onClick={onCopy}> | |
| Copy | |
| </ContextMenuItem>, | |
| <ContextMenuItem | |
| disabled={!isCopying} | |
| data-testid="context-menu-delete" | |
| onClick={onPaste} | |
| > | |
| Paste | |
| </ContextMenuItem>, | |
| <ContextMenuItem data-testid="context-menu-copy" onClick={onCopy}> | |
| Copy | |
| </ContextMenuItem>, | |
| <ContextMenuItem | |
| disabled={!isCopying} | |
| data-testid="context-menu-paste" | |
| onClick={onPaste} | |
| > | |
| Paste | |
| </ContextMenuItem>, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
relates #7952
Issue
duplicate/clonewas not fully recursive in the original file tree.Implementation
fs.copyon asrcandtargetScreenshots
Will work with other file extensions