Refactor: Optimize ContextMenu.vue by replacing timers with Vue lifecycle methods #486
Conversation
WalkthroughThe pull request refactors the context menu component in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CM as ContextMenu Component
participant AA as Action Array
U->>CM: Trigger context menu (e.g., right-click)
CM->>CM: Call showContextMenu()
CM-->>U: Render menu with contextMenuStyle changes
U->>CM: Click menu item (pass index)
CM->>AA: Invoke action at index
AA-->>CM: Return action response
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ContextMenu/ContextMenu.vue (1)
42-50: Consider extracting transition duration to CSS.The computed property effectively handles the menu's visibility state. However, the transition timing could be moved to CSS for better maintainability.
Consider this approach:
computed: { contextMenuStyle() { return { opacity: this.ctxPos.visible ? '1' : '0', visibility: this.ctxPos.visible ? 'visible' : 'hidden', - transition: 'opacity 0.2s ease-in-out', }; }, },Add to your component's style section:
<style> #contextMenu { transition: opacity 0.2s ease-in-out; } </style>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ContextMenu/ContextMenu.vue(2 hunks)
🔇 Additional comments (2)
src/components/ContextMenu/ContextMenu.vue (2)
2-2: LGTM! Template changes align with Vue best practices.The changes improve the code by:
- Using Vue's reactivity system with v-show and computed styles instead of manual DOM manipulation
- Simplifying the click handler by passing the index directly
Also applies to: 8-8
57-62: LGTM! Methods are now simpler and more focused.The visibility methods are now cleaner and more maintainable.
|
@JoshVarga @Arnabdaz PTAL |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ContextMenu/ContextMenu.vue (1)
42-50: Consider making transition duration configurable.The computed property effectively handles menu visibility with smooth transitions.
Consider extracting the hardcoded transition duration to a prop or constant:
contextMenuStyle() { return { opacity: this.ctxPos.visible ? '1' : '0', visibility: this.ctxPos.visible ? 'visible' : 'hidden', - transition: 'opacity 0.2s ease-in-out', + transition: `opacity ${this.transitionDuration}s ease-in-out`, }; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ContextMenu/ContextMenu.vue(2 hunks)
🔇 Additional comments (3)
src/components/ContextMenu/ContextMenu.vue (3)
2-2: LGTM! Vue-idiomatic template changes.The changes improve the component by:
- Using v-show with computed styles for better reactivity
- Passing the index directly to the click handler for improved type safety
Also applies to: 8-8
37-39: Verify context menu positioning logic.The x,y coordinates have been removed from ctxPos, but there's no visible logic for positioning the context menu relative to the click location.
Please confirm how the context menu is positioned on the screen. You might need to:
- Add CSS positioning (fixed/absolute)
- Add logic to update menu position on right-click
57-62: LGTM! Clean visibility management.The simplified visibility methods align well with Vue's reactivity system and eliminate the need for timers.
| async menuItemClicked(index) { | ||
| this.hideContextMenu() | ||
| const id = event.target.dataset.index | ||
| if (id == 0) { | ||
| document.execCommand('copy') | ||
| } else if (id == 1) { | ||
| document.execCommand('cut') | ||
| } else if (id == 2) { | ||
| // document.execCommand('paste'); it is restricted to sove this problem we use dataPasted variable | ||
| paste(localStorage.getItem('clipboardData')) | ||
| } else if (id == 3) { | ||
| deleteSelected() | ||
| } else if (id == 4) { | ||
| undo() | ||
| } else if (id == 5) { | ||
| createNewCircuitScope() | ||
| } else if (id == 6) { | ||
| logixFunction.createSubCircuitPrompt() | ||
| } else if (id == 7) { | ||
| globalScope.centerFocus(false) | ||
| } | ||
| try { | ||
| switch (index) { | ||
| case 0: // Copy | ||
| if (navigator.clipboard) { | ||
| const text = window.getSelection().toString(); | ||
| if (text) { | ||
| await navigator.clipboard.writeText(text); | ||
| console.log("Copied:", text); | ||
| } else { | ||
| console.warn("No text selected for copying."); | ||
| } | ||
| } else { | ||
| console.error("Clipboard API not available."); | ||
| } | ||
| break; | ||
|
|
||
| case 1: // Cut | ||
| if (navigator.clipboard) { | ||
| const text = window.getSelection().toString(); | ||
| if (text) { | ||
| await navigator.clipboard.writeText(text); | ||
| document.execCommand('delete'); | ||
| console.log("Cut:", text); | ||
| } else { | ||
| console.warn("No text selected for cutting."); | ||
| } | ||
| } else { | ||
| console.error("Clipboard API not available."); | ||
| } | ||
| break; | ||
|
|
||
| case 2: // Paste | ||
| const clipboardData = await navigator.clipboard.readText(); | ||
| if (clipboardData) { | ||
| paste(clipboardData); | ||
| console.log("Pasted:", clipboardData); | ||
| } else { | ||
| console.warn("No clipboard data available."); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| console.warn("No action defined for this menu option."); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error executing menu action:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement missing menu actions.
The switch statement only handles Copy/Cut/Paste (cases 0-2) but ignores:
- Delete (case 3)
- Undo (case 4)
- New Circuit (case 5)
- Insert Subcircuit (case 6)
- Center Focus (case 7)
Add the missing cases:
switch (index) {
case 0: // Copy
// ... existing code ...
case 1: // Cut
// ... existing code ...
case 2: // Paste
// ... existing code ...
+ case 3: // Delete
+ deleteSelected();
+ break;
+ case 4: // Undo
+ undo();
+ break;
+ case 5: // New Circuit
+ createNewCircuitScope();
+ break;
+ case 6: // Insert Subcircuit
+ logixFunction.createSubCircuitPrompt();
+ break;
+ case 7: // Center Focus
+ if (typeof globalScope === 'undefined') {
+ console.error('globalScope is not defined');
+ return;
+ }
+ globalScope.centerFocus(false);
+ break;
default:
console.warn("No action defined for this menu option.");
}Let's verify the globalScope reference:
🏁 Script executed:
#!/bin/bash
# Search for globalScope definition
echo "Searching for globalScope in JavaScript files:"
rg "globalScope" --type js
echo "Searching for globalScope in Vue files:"
rg "globalScope" -g "*.vue"Length of output: 66190
Action Required: Implement Missing Menu Cases in ContextMenu.vue
The globalScope reference is valid and widely used throughout the codebase, so the check in case 7 is appropriate. However, the switch statement currently only supports Copy (0), Cut (1), and Paste (2). Please add the missing actions as follows:
- Case 3 – Delete: Call
deleteSelected(). - Case 4 – Undo: Invoke
undo(). - Case 5 – New Circuit: Trigger
createNewCircuitScope(). - Case 6 – Insert Subcircuit: Use
logixFunction.createSubCircuitPrompt(). - Case 7 – Center Focus: Confirm
globalScopeis defined before callingglobalScope.centerFocus(false).
switch (index) {
case 0: // Copy
// ... existing code ...
case 1: // Cut
// ... existing code ...
case 2: // Paste
// ... existing code ...
+ case 3: // Delete
+ deleteSelected();
+ break;
+ case 4: // Undo
+ undo();
+ break;
+ case 5: // New Circuit
+ createNewCircuitScope();
+ break;
+ case 6: // Insert Subcircuit
+ logixFunction.createSubCircuitPrompt();
+ break;
+ case 7: // Center Focus
+ if (typeof globalScope === 'undefined') {
+ console.error('globalScope is not defined');
+ return;
+ }
+ globalScope.centerFocus(false);
+ break;
default:
console.warn("No action defined for this menu option.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async menuItemClicked(index) { | |
| this.hideContextMenu() | |
| const id = event.target.dataset.index | |
| if (id == 0) { | |
| document.execCommand('copy') | |
| } else if (id == 1) { | |
| document.execCommand('cut') | |
| } else if (id == 2) { | |
| // document.execCommand('paste'); it is restricted to sove this problem we use dataPasted variable | |
| paste(localStorage.getItem('clipboardData')) | |
| } else if (id == 3) { | |
| deleteSelected() | |
| } else if (id == 4) { | |
| undo() | |
| } else if (id == 5) { | |
| createNewCircuitScope() | |
| } else if (id == 6) { | |
| logixFunction.createSubCircuitPrompt() | |
| } else if (id == 7) { | |
| globalScope.centerFocus(false) | |
| } | |
| try { | |
| switch (index) { | |
| case 0: // Copy | |
| if (navigator.clipboard) { | |
| const text = window.getSelection().toString(); | |
| if (text) { | |
| await navigator.clipboard.writeText(text); | |
| console.log("Copied:", text); | |
| } else { | |
| console.warn("No text selected for copying."); | |
| } | |
| } else { | |
| console.error("Clipboard API not available."); | |
| } | |
| break; | |
| case 1: // Cut | |
| if (navigator.clipboard) { | |
| const text = window.getSelection().toString(); | |
| if (text) { | |
| await navigator.clipboard.writeText(text); | |
| document.execCommand('delete'); | |
| console.log("Cut:", text); | |
| } else { | |
| console.warn("No text selected for cutting."); | |
| } | |
| } else { | |
| console.error("Clipboard API not available."); | |
| } | |
| break; | |
| case 2: // Paste | |
| const clipboardData = await navigator.clipboard.readText(); | |
| if (clipboardData) { | |
| paste(clipboardData); | |
| console.log("Pasted:", clipboardData); | |
| } else { | |
| console.warn("No clipboard data available."); | |
| } | |
| break; | |
| default: | |
| console.warn("No action defined for this menu option."); | |
| } | |
| } catch (error) { | |
| console.error("Error executing menu action:", error); | |
| } | |
| } | |
| async menuItemClicked(index) { | |
| this.hideContextMenu() | |
| try { | |
| switch (index) { | |
| case 0: // Copy | |
| if (navigator.clipboard) { | |
| const text = window.getSelection().toString(); | |
| if (text) { | |
| await navigator.clipboard.writeText(text); | |
| console.log("Copied:", text); | |
| } else { | |
| console.warn("No text selected for copying."); | |
| } | |
| } else { | |
| console.error("Clipboard API not available."); | |
| } | |
| break; | |
| case 1: // Cut | |
| if (navigator.clipboard) { | |
| const text = window.getSelection().toString(); | |
| if (text) { | |
| await navigator.clipboard.writeText(text); | |
| document.execCommand('delete'); | |
| console.log("Cut:", text); | |
| } else { | |
| console.warn("No text selected for cutting."); | |
| } | |
| } else { | |
| console.error("Clipboard API not available."); | |
| } | |
| break; | |
| case 2: // Paste | |
| const clipboardData = await navigator.clipboard.readText(); | |
| if (clipboardData) { | |
| paste(clipboardData); | |
| console.log("Pasted:", clipboardData); | |
| } else { | |
| console.warn("No clipboard data available."); | |
| } | |
| break; | |
| case 3: // Delete | |
| deleteSelected(); | |
| break; | |
| case 4: // Undo | |
| undo(); | |
| break; | |
| case 5: // New Circuit | |
| createNewCircuitScope(); | |
| break; | |
| case 6: // Insert Subcircuit | |
| logixFunction.createSubCircuitPrompt(); | |
| break; | |
| case 7: // Center Focus | |
| if (typeof globalScope === 'undefined') { | |
| console.error('globalScope is not defined'); | |
| return; | |
| } | |
| globalScope.centerFocus(false); | |
| break; | |
| default: | |
| console.warn("No action defined for this menu option."); | |
| } | |
| } catch (error) { | |
| console.error("Error executing menu action:", error); | |
| } | |
| } |
Fixes #469
Describe the changes you have made in this PR -
This PR refactors ContextMenu.vue to replace setTimeout with Vue lifecycle hooks, improving performance and reducing unnecessary browser resource usage.
Why this change?
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
showContextMenu, has been added to control the visibility of the context menu.Refactor