feat: Add note list context menu#376
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 26 medium |
| BestPractice | 7 medium 3 high |
| ErrorProne | 11 medium 3 high |
🟢 Metrics 1126 complexity · 11 duplication
Metric Results Complexity 1126 Duplication 11
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
While this PR aims to add a necessary context menu to the note list, it currently falls short of quality and verification standards. Although the overall Codacy analysis is 'up to standards', the metadata reveals 32 new issues in NoteContextMenu.tsx and extreme complexity scores in App.tsx (717) and useNoteListModel.tsx (307). Furthermore, none of the required acceptance criteria for the context menu (triggering, action invocation, or stateful labels) have been verified with tests. These quality and coverage gaps should be addressed before merging.
About this PR
- There is a total lack of regression and functional coverage for the new context menu. All 5 identified test scenarios (right-click trigger, action visibility, callback invocation, etc.) are missing from the implementation.
- The PR introduces significant technical debt:
NoteContextMenu.tsxcontains 32 new issues, andApp.tsxhas reached an extremely high complexity score of 717. These files should be refactored to ensure maintainability.
Test suggestions
- Context menu triggers on right-click of a note row
- Menu displays all required actions (note, retargeting, file, archive, delete)
- Changes view still displays discard/restore menu correctly
- Clicking a menu action invokes the correct row-specific callback
- Menu labels update correctly based on state (e.g. Archive vs Unarchive)
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Context menu triggers on right-click of a note row
2. Menu displays all required actions (note, retargeting, file, archive, delete)
3. Changes view still displays discard/restore menu correctly
4. Clicking a menu action invokes the correct row-specific callback
5. Menu labels update correctly based on state (e.g. Archive vs Unarchive)
Low confidence findings
- There appears to be a discrepancy or technical issue where code diffs were not accessible to the review agents; this prevents a granular logic verification of the context menu implementation.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
There was a problem hiding this comment.
Pull Request Overview
The pull request is currently not up to standards, primarily due to high complexity in core logic and several functional gaps in the context menu implementation.
Key issues that should prevent merging include:
- Missing dismissal logic (Escape key/outside clicks) which was a defined acceptance criterion.
- Lack of viewport collision detection, which will cause the menu to render off-screen in common UI positions.
- Significant architectural debt in
useNoteListModel.tsx, which has reached a complexity score that threatens maintainability. - Several features (Changes view prioritization and specific disabling logic for retargeting/markdown files) lack verification and appear unaddressed in the current implementation.
About this PR
- The implementation logic for the Changes view menu in 'useNoteListModel.tsx' may result in the loss of standard note actions while in that view. Please verify if this exclusion is intentional or if the new note actions should be merged with the specialized 'Discard/Restore' logic.
- The new context menu implementation lacks standard accessibility markers. Ensure all new UI elements follow W3C WAI-ARIA patterns for menus to support assistive technologies.
1 comment outside of the diff
src/components/note-list/useNoteListModel.tsx
line 575🔴 HIGH RISK
TheuseNoteListModelhook has reached critical complexity (delta 307) and takes 42 parameters. Consider refactoring by grouping related dependencies intoNoteListStateandNoteListActionsobjects, and extracting the context menu logic into a separate hook.
Test suggestions
- Context menu opens on right-click and displays all expected primary actions
- Menu items trigger their respective callbacks with the correct entry or path
- Labels update dynamically based on the note's state (e.g., favorite, archived)
- Menu dismisses automatically on Escape key press or clicking outside
- The Changes view prioritizes the specialized changes menu over the generic note menu
- The 'Open in Default App' action is correctly disabled for markdown files
- Retargeting actions are disabled when the entry cannot be moved or changed
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Menu dismisses automatically on Escape key press or clicking outside
2. The Changes view prioritizes the specialized changes menu over the generic note menu
3. The 'Open in Default App' action is correctly disabled for markdown files
4. Retargeting actions are disabled when the entry cannot be moved or changed
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| { | ||
| icon: ArrowSquareOut, | ||
| label: 'Open in Default App', | ||
| onSelect: () => runAndClose(() => actions.onOpenExternalFile?.(entry.path), onClose), |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This label is hardcoded, which prevents localization. Use the 'translate' function with an appropriate i18n key.
| onSelect: () => runAndClose(() => actions.onOpenExternalFile?.(entry.path), onClose), | |
| label: translate(locale, 'command.note.openExternalApp'), |
| > | ||
| {groups.map((items, index) => ( | ||
| <div key={items[0]?.testId ?? index}> | ||
| {index > 0 && <div className="my-1 h-px bg-border" role="separator" />} |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Use a native
element instead of a
| {index > 0 && <div className="my-1 h-px bg-border" role="separator" />} | |
| {index > 0 && <hr className="my-1 border-t border-border" />} |
| <div | ||
| ref={menuRef} | ||
| className="fixed z-50 rounded-md border bg-popover p-1 shadow-md" | ||
| style={{ left: menu.x, top: menu.y, minWidth: 220 }} |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The menu may appear partially off-screen if opened near the bottom-right corner of the window. Add viewport collision detection logic to adjust the left and top coordinates so the menu remains fully visible.
| { | ||
| icon: FolderOpen, | ||
| label: translate(locale, 'editor.toolbar.revealFile'), | ||
| onSelect: () => runAndClose(() => actions.onRevealFile?.(entry.path), onClose), |
There was a problem hiding this comment.
⚪ LOW RISK
The return value of this arrow function is void. Add braces to the function body to clarify that no value is being returned.
| onSelect: () => runAndClose(() => actions.onRevealFile?.(entry.path), onClose), | |
| onSelect: () => { runAndClose(() => actions.onRevealFile?.(entry.path), onClose); }, |
Summary
Tests