feat(editor): Add "Copy Log Event" option to context menu (resolves #179). #197
Conversation
WalkthroughThe update enhances the Monaco editor's custom action setup and introduces a feature for copying log events. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as Editor
participant D as Document
U->>E: Select a log event line
E->>E: Execute getSelectedLogEventNum()
E->>U: Return log event number (or null)
U->>E: Trigger COPY_LOG_EVENT action via context menu
E->>E: Call handleCopyLogEventAction()
E->>E: Set selection range on log event line(s)
E->>D: Execute document.execCommand("copy")
D-->>E: Confirm text copied to clipboard
Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Editor/index.tsx (2)
72-120: Consider using the modern Clipboard API instead of the deprecated execCommand.While the current implementation works,
document.execCommand("copy")is deprecated. Consider using the Clipboard API for better future compatibility:- // Monaco editor uses `document.execCommand` instead of the Clipboard API to copy text. - // eslint-disable-next-line @typescript-eslint/no-deprecated - editor.getContainerDomNode().ownerDocument.execCommand("copy"); + // Get the selected text from the editor + const selectedText = editor.getModel()?.getValueInRange(editor.getSelection()); + + // Use the Clipboard API with fallback to execCommand + if (selectedText && navigator.clipboard) { + navigator.clipboard.writeText(selectedText).catch(() => { + // Fallback if clipboard API fails + // eslint-disable-next-line @typescript-eslint/no-deprecated + editor.getContainerDomNode().ownerDocument.execCommand("copy"); + }); + } else { + // Fallback for browsers without Clipboard API + // eslint-disable-next-line @typescript-eslint/no-deprecated + editor.getContainerDomNode().ownerDocument.execCommand("copy"); + }Also, consider adding user feedback (like a toast notification) to indicate successful copying.
108-113: Ensure endLineNumber is always valid.There's a potential edge case where
maxLineNum - 1could result in a negative number if the document is empty (though unlikely). Consider adding a safeguard:const maxLineNum: number = model.getLineCount(); const startLineNumber: number = hoveredLogEventLineNum; const endLineNumber: number = null === nextLogEventLineNum ? - maxLineNum - 1 : + Math.max(maxLineNum - 1, startLineNumber) : nextLogEventLineNum - 1;This ensures that even in edge cases, endLineNumber is always at least equal to startLineNumber.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Editor/MonacoInstance/actions.ts(1 hunks)src/components/Editor/index.tsx(2 hunks)src/utils/actions.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/utils/actions.tssrc/components/Editor/MonacoInstance/actions.tssrc/components/Editor/index.tsx
🔇 Additional comments (8)
src/utils/actions.ts (3)
15-15: Good addition of a new action to copy log events.The enum name is clear and follows the existing convention for naming actions.
22-22: Good enhancement to the EditorAction interface.The optional contextMenuGroupId property allows for better organization of context menu items.
65-70: Well-defined new action for copying log events.The action is properly configured with:
- Descriptive label that clearly communicates the purpose
- Appropriate key bindings (Ctrl/Cmd+Shift+C)
- Context menu grouping that helps organize the right-click menu
This implementation aligns well with the PR objective of adding a menu item to copy a single log event.
src/components/Editor/MonacoInstance/actions.ts (3)
158-158: Good update to destructure the new contextMenuGroupId property.The function signature update maintains backward compatibility since the property is optional.
162-169: Good refactoring to use a descriptor object.Creating a separate descriptor object improves code readability compared to passing an inline object to editor.addAction.
171-174: Proper conditional handling of the contextMenuGroupId property.The type check using
"undefined" !== typeof contextMenuGroupIdfollows the coding guidelines preference for explicit comparison rather than using the negation operator.src/components/Editor/index.tsx (2)
42-70: Well-implemented helper function to identify hovered log events.The function is well-documented with JSDoc comments and includes proper null/undefined checks. It correctly uses the utility function to find the corresponding log event number based on line position.
163-167: Good implementation of the COPY_LOG_EVENT action handler.The case properly calls the new handler function with the required parameters and follows the pattern of existing action handlers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Editor/MonacoInstance/actions.ts(1 hunks)src/components/Editor/index.tsx(2 hunks)src/utils/actions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Editor/index.tsx
- src/components/Editor/MonacoInstance/actions.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/utils/actions.ts
🔇 Additional comments (3)
src/utils/actions.ts (3)
15-15: LGTM! Well-integrated enum addition for the new copy log event action.The new
COPY_LOG_EVENTenum value is clearly named and follows the same pattern as existing enum values.
20-21: LGTM! Type definitions properly implemented.The new interface properties use the
Nullable<string>andNullable<number>types correctly, which is preferable to using the optional property syntax with?as it makes the intent clearer.
66-72: LGTM! Well-configured action for copying log events.The new action is well-structured and includes appropriate keyboard shortcuts for the copy log event action. Using
contextMenuGroupId: "9_cutcopypaste"puts this action in the same group as other copy/paste actions, which is good for discoverability.As discussed in a previous review, putting the "Copy Log Event" button in the same group as the standard "Copy" button (using "9_cutcopypaste") makes sense from a user experience perspective.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Editor/index.tsx (2)
72-121: Good implementation with a minor consideration for modern clipboard API.The implementation correctly handles copying the selected log event to the clipboard, including proper edge case handling. It calculates the start and end of the log event properly, sets the selection in the editor, and then initiates the copy operation.
While the code uses the deprecated
document.execCommand("copy")API (with a proper comment explaining why), consider evaluating if the Clipboard API could be used as a more modern alternative in a future update:// Future consideration (not required for this PR) navigator.clipboard.writeText(model.getValueInRange({ startLineNumber: startLineNumber, startColumn: 1, endLineNumber: endLineNumber, endColumn: endMaxColumn }));This is just a suggestion for future improvements, as noted in previous review comments where it was discussed and decided to stick with the current approach.
104-113: Consider simplifying line range calculation using Math.min.The logic for calculating the end line number is correct, but could be simplified.
- const endLineNumberMaybeNegative: number = null === nextLogEventLineNum ? - maxLineNum - 1 : - nextLogEventLineNum - 1; - const endLineNumber: number = Math.max(startLineNumber, endLineNumberMaybeNegative); + const endLineNumber: number = null === nextLogEventLineNum ? + maxLineNum : + Math.max(startLineNumber, nextLogEventLineNum - 1);This change reduces the need for the variable
endLineNumberMaybeNegativeand makes the intention clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Editor/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/Editor/index.tsx
🔇 Additional comments (3)
src/components/Editor/index.tsx (3)
42-70: Function name should be more descriptive of its action.The function name
getSelectedLogEventNumis somewhat misleading as it retrieves the log event number from a mouse position in the editor, rather than from an actual selection. Based on the previous review comments, there was a discussion about naming this function, andgetSelectedLogEventNumwas agreed upon instead ofgetHoveredLogEventNum.The implementation itself is good and follows the proper coding guidelines with the
false ==pattern on line 54 rather than using the negation operator.
164-168: Action handler integrated properly.The handler for the new
COPY_LOG_EVENTaction is correctly implemented. It retrieves the current mapping from the ref and calls the appropriate function.
42-121: Verify multi-event selection handling.As discussed in previous review comments, handling the case where a user has selected multiple lines across different log events when clicking the "Copy Log Event" button is planned for a future PR. This current implementation only copies a single log event regardless of the existing selection.
This is acceptable for the current PR as agreed upon, but should be tracked for future enhancement.
|
For the PR title, how about |
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
hoophalab
changed the title
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/utils/actions.ts (1)
19-23:⚠️ Potential issueAdd default values to existing EditorAction objects
The interface now requires new properties
contextMenuGroupIdandcontextMenuOrder, but the existing action objects inEDITOR_ACTIONSdon't specify these properties. This mismatch could cause TypeScript to generate errors.Apply this change to all existing action objects in the array:
{ actionName: null, label: "Focus on Editor", keyBindings: [monaco.KeyCode.Backquote], + contextMenuGroupId: null, + contextMenuOrder: null, },This should be done for all existing action objects (lines 31-65).
🧹 Nitpick comments (2)
src/components/Editor/index.tsx (2)
42-70: Function to retrieve log event number is well-implementedThe function correctly retrieves the log event number based on the currently selected line in the editor. Error handling is appropriate for cases where no selection exists or when the mapping is unavailable.
However, for better naming consistency, consider renaming the parameter:
const getSelectedLogEventNum = ( editor: monaco.editor.IStandaloneCodeEditor, - beginLineNumToLogEventNumRefCurrent: BeginLineNumToLogEventNumMap + beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap ): Nullable<number> => {This would align with the parameter naming in other functions and make the code more readable.
90-98: Improve null handling for better error reportingThe "unreachable" comment suggests that this case shouldn't happen, but robust code should still handle potential errors explicitly rather than silently returning.
if (null === selectedLogEventLineNum) { - // unreachable - return; + throw new Error("Selected log event line number is null despite having a valid log event number."); }This would provide better diagnostics if this "unreachable" code is ever reached in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Editor/index.tsx(2 hunks)src/utils/actions.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/utils/actions.tssrc/components/Editor/index.tsx
🔇 Additional comments (3)
src/utils/actions.ts (2)
15-15: New enum value added for log event copyingThis addition to the enum is well-defined and follows the naming convention established in the rest of the enum.
66-72: Copy Log Event action defined with appropriate parametersThe new action for copying log events is well-defined with all necessary properties. The action is assigned to the "9_cutcopypaste" context menu group with order 2, which should place it appropriately in the context menu next to other copy/paste operations.
src/components/Editor/index.tsx (1)
164-166: Action handler implementation correctly dispatches to copy functionThe implementation of the COPY_LOG_EVENT case in the switch statement is correct and follows the pattern established for other actions.
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Editor/index.tsx (1)
64-112: Enhance error handling for the next log event line numberThe current implementation has thorough error handling for
selectedLogEventLineNumbut doesn't explicitly check ifnextLogEventLineNumis null before using it in the conditional branch that assumes it exists.Consider implementing a more consistent error handling approach:
if (null === selectedLogEventLineNum) { throw new Error("Unable to get the beginning line number of the selected log event."); } let endLineNumber: number; if (null !== nextLogEventLineNum) { endLineNumber = nextLogEventLineNum - 1; } else { + // Handle the case when this is the last log event in the file const model = editor.getModel(); if (null === model) { throw new Error("Unable to get the text model."); } endLineNumber = model.getLineCount() - 1; }The added comment helps clarify the else branch's purpose for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Editor/MonacoInstance/actions.ts(1 hunks)src/components/Editor/index.tsx(2 hunks)src/utils/actions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Editor/MonacoInstance/actions.ts
- src/utils/actions.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/Editor/index.tsx
🔇 Additional comments (3)
src/components/Editor/index.tsx (3)
42-62: Good implementation of selected log event detection!The function is well-documented with clear JSDoc comments and properly handles the case when no line is selected. The implementation correctly uses the
getMapValueWithNearestLessThanOrEqualKeyutility function to find the corresponding log event number.
103-111: Good use of Monaco's built-in clipboard action!The implementation correctly uses Monaco's built-in clipboard action instead of the deprecated
document.execCommandAPI. Setting the selection and then triggering the copy action makes the operation more intuitive for users, as they can see what's being copied.
155-157: Proper integration of the copy action in the editor's action handlerThe new case for handling the copy log event action is correctly implemented and properly integrated with the existing action handler.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Editor/index.tsx (2)
64-90: Consider consolidating error handling for both null checksThe function properly throws an error when
selectedLogEventLineNumis null, but handles thenextLogEventLineNumbeing null differently. Consider using similar error handling approaches for both cases for greater consistency.if (null === selectedLogEventLineNum) { throw new Error("Unable to get the beginning line number of the selected log event."); }The throw error approach is good for catching programming issues, but consider enhancing the error message to be more actionable, particularly for debugging purposes.
92-94: Prefer Yoda conditions for boolean comparisonsAccording to the coding guidelines, prefer
false == <expression>rather than!<expression>. For consistency, you should also apply this pattern to null checks.-if (null !== nextLogEventLineNum) { +if (null === nextLogEventLineNum) { + // Handle the case when this is the last log event in the file. + const model = editor.getModel(); + if (null === model) { + throw new Error("Unable to get the text model."); + } + endLineNumber = model.getLineCount() - 1; +} else { endLineNumber = nextLogEventLineNum - 1; -} else { - // Handle the case when this is the last log event in the file. - const model = editor.getModel(); - if (null === model) { - throw new Error("Unable to get the text model."); - } - endLineNumber = model.getLineCount() - 1; }This maintains the Yoda condition style while keeping the same logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Editor/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/Editor/index.tsx
🔇 Additional comments (4)
src/components/Editor/index.tsx (4)
42-62: Function is well-implemented and follows naming recommendationsThe implementation of
getSelectedLogEventNumis clean and follows the naming convention agreed upon in previous discussions. It properly handles undefined line numbers and uses appropriate utility functions to find the log event number.
91-102: Good handling of the last log event edge caseThe code properly handles the case when processing the last log event in the file by retrieving the model's line count. This is a good edge case consideration.
104-113: Excellent use of Monaco's clipboard APIThe implementation uses Monaco's recommended clipboard action rather than deprecated methods, which is a best practice. Using
Infinityfor the column position is also a good approach to ensure selection to the end of each line.
156-158: Action handler is correctly integratedThe new case for
ACTION_NAME.COPY_LOG_EVENTis properly integrated into the existing switch statement and correctly passes required parameters to the handler function.
There was a problem hiding this comment.
good to be merged once we address https://github.com/y-scope/yscope-log-viewer/pull/197/files#r1982271537
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Editor/index.tsx (1)
64-90: Consider adding error handling for the case whennextLogEventLineNumis nullCurrently, you check if
selectedLogEventLineNumis null and throw an error, but there's no explicit check fornextLogEventLineNumbeing null before it's used in the next section. Even though this might be addressed by the null check in the if-statement on line 93, it would be more explicit to validate both values at the same time.if (null === selectedLogEventLineNum) { throw new Error("Unable to get the beginning line number of the selected log event."); } + +if (null === nextLogEventLineNum && null === selectedLogEventLineNum + 1) { + // This is expected when handling the last log event + console.debug("This is the last log event in the file."); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Editor/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/Editor/index.tsx
🔇 Additional comments (3)
src/components/Editor/index.tsx (3)
42-62: Clear and well-implemented function for retrieving selected log event numberThe function logic is clear, handles edge cases correctly, and returns the appropriate log event number based on the cursor position. The function documentation is comprehensive and accurate.
91-113: Good handling of the last log event case and use of Monaco editor's built-in clipboard actionThis section:
- Properly handles the edge case when copying the last log event
- Correctly sets the selection range
- Uses Monaco's built-in clipboard action instead of deprecated APIs
The Infinity value for the end column ensures the entire line is selected, which is a neat solution.
156-158: Implementation follows the established pattern for editor custom actionsThe new action handler is concise and follows the same pattern as other action handlers, using the current reference to beginLineNumToLogEventNum.
…fixes #179).
Description
Add a context menu item to allow users to easily select and copy a single log event when it spans multiple lines.
Checklist
breaking change.
Validation performed
Right-clicking on any line in the editor and selecting the "Copy log event" item selects the entire log event and copys it to the clipboard.
Summary by CodeRabbit
Summary by CodeRabbit