feat : add right-click menu#397
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 6 medium |
| BestPractice | 6 medium 1 high |
| ErrorProne | 4 medium |
| Security | 1 high |
| Complexity | 4 medium |
🟢 Metrics 1289 complexity · 16 duplication
Metric Results Complexity 1289 Duplication 16
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR is currently below quality standards. Critical issues include an object injection vulnerability in FolderTree.tsx and missing button types that could trigger unintended form submissions. There is also a functional discrepancy: the implementation adds a 'View' option which was not mentioned in the requirements, while 'Tags' appears to have been omitted or renamed.
Maintainability is a significant concern as App.tsx and Sidebar.tsx have exceeded acceptable complexity levels. The PR introduces a fragile communication pattern in FolderTree.tsx using side-effects (setTimeout) to trigger UI forms across components. These architectural and security flaws should be resolved before approval.
About this PR
- The PR introduces a side-effect-driven approach to cross-component communication (using 'createRequestKey' and 'setTimeout') which is fragile and difficult to trace. Additionally, several core files (App.tsx, Sidebar.tsx, FolderTree.tsx) have reached cyclomatic complexity levels that make them high-risk for future modifications. A structural refactor to extract vault handlers into hooks and simplify the layout is strongly recommended.
3 comments outside of the diff
src/App.tsx
line 1🟡 MEDIUM RISK
Suggestion: This file has grown to 1479 lines with extreme cyclomatic complexity. Extract vault-related handlers into a custom hook and separate the layout into smaller functional components to improve testability and maintainability.
src/components/sidebar/SidebarSections.tsx
line 280🔴 HIGH RISK
Add an explicit type='button' to prevent accidental form submissions and ensure consistent accessibility behavior.
src/mock-tauri/mock-handlers.ts
line 259🔴 HIGH RISK
Use Object.hasOwn(MOCK_CONTENT, newPath) for a more idiomatic and safe property existence check.
Test suggestions
- Right-clicking the sidebar background (empty space) displays the creation menu.
- Background menu actions (Create View, Create Type) trigger their respective callbacks.
- Folder context menu displays the 'Create subfolder' action when a handler is provided.
- Selecting 'Create subfolder' renders an inline input field under the correct parent folder.
- The backend create_vault_folder command correctly handles and validates nested relative paths (e.g., 'parent/child').
- The backend rejects folder paths containing invalid components like RootDir or ParentDir.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const handleCreateChildFolderFromMenu = useCallback((folderPath: string) => { | ||
| closeContextMenu() | ||
| setCreatingChildParentPath(folderPath) | ||
| if (!expanded[folderPath]) toggleFolder(folderPath) |
There was a problem hiding this comment.
🔴 HIGH RISK
To prevent potential object injection vulnerabilities when using dynamic folder paths as keys, consider using a Map or checking for property ownership with Object.hasOwn.
| const timeoutId = window.setTimeout(handleCreateFolderClick, 0) | ||
| return () => window.clearTimeout(timeoutId) | ||
| }, [createRequestKey, handleCreateFolderClick]) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The use of 'createRequestKey' and 'setTimeout' to trigger the create form is a side-effect-driven pattern that is fragile. If this deferral is required to handle focus or context menu closure, it should be documented. Additionally, avoid returning the void result of 'clearTimeout' in the shorthand arrow function as it is misleading.
| ? `${parentPath.replace(/^\/+|\/+$/g, '')}/${name}` | ||
| : name |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Consider cleaning the folder name to avoid accidental double slashes if the user input contains a leading slash during path construction.
| {canCreateView && ( | ||
| <Button type="button" variant="ghost" className={buttonClass} onClick={onCreateView}> | ||
| <ListPlus size={15} /> | ||
| {translate(locale, 'sidebar.action.createView')} | ||
| </Button> |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The menu implementation includes 'Create view' which was not mentioned in the PR description, while 'Tags' appears to have been replaced by 'Types'. Please verify if 'View' is a requested addition and ensure UI labels align with the specification.
Since my knowledge base heavily relies on folders for hierarchical management, I just develop:
2 Users can right-click an exists folder to create a subfolder
If this feature is approved, I would like to proceed to the next step and implement more detailed functions to create and move files within folders