Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new-log-viewer: Integrate Monaco Editor for enhanced log viewing. #54

Merged
merged 37 commits into from
Aug 24, 2024

Conversation

junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Aug 17, 2024

References

new-log-viewer series: #45 #46 #48 #51 #52 #53

Description

This PR introduces the MonacoInstance component, integrating the Monaco Editor into the log viewer application. The editor provides a more interactive and feature-rich environment for viewing log files, including custom actions and mobile support.

  1. MonacoInstance Component:
    • monaco-editor Wrapper (named <MonacoEdior/>): A React component that wraps the official monaco-editor with configurable properties and event handlers.
    • <MonacoEdior/> Wrapper (named <MonacoInstance/>; see index.tsx): A React component that wraps the <MonacoEditor/> with logging viewing specific context and states.
  2. Monaco Editor Integration into Layout: Now the logs messages are replaced by the newly added <MonacoInstance/>.
  3. Utility Enhancements:
    • Added a new data structure utility function getMapKeyByValue to find the key associated with a specific value in a map.
    • Added math utilities getLastItemNumInPrevChunk and getNextItemNumInNextChunk.
  4. CSS Refactor: Consolidated CSS selectors for full-screen layout configuration.

Validation performed

  1. Referred to the validation steps in Add support for loading files, decoding them as JSONL, and formatting them using a Logback-like format string. #46 . Loaded example.jsonl.
  2. Observed the logs being presented in the newly added MonacoInstance which takes the full width and rest of height of the viewport.
  3. Clicked debug button "Clear localStorage" to ensure the format string and page size (default: 10000) settings are restored to default.
  4. On Page 1, clicked on the 3rd line (the 3rd log message) and observed the logEventNum being updated in both the debug header and the URL parameter as 3.
  5. Updated the logEventNum as 1 in the debug header input and observed the cursor moved to the first event; updated the logEventNum in the URL hash parameter and observed the cursor moved to the 3rd event.
  6. Pressed Ctrl + ] (shortcut for "Next Page" action) and observed the 2nd page getting loaded. The cursor position was at the first event on the page (logEventNum=10001).
  7. Now on Page 2, clicked on the 3rd line (the 10003rd log message) and observed the logEventNum being updated in both the debug header and the URL parameter as 10003.
  8. Pressed Ctrl + [ (shortcut for "Previous Page" action) and observed the 1st page getting loaded. The cursor position was at the last event on the page (logEventNum=10000).
  9. Now on Page 1, pressed Ctrl + [ again and the cursor moved to the very first message.
  10. Pressed Ctrl + I (shortcut for "Go to Bottom") and observed the cursor moved to the last line and the last line got revealed in the editor.
  11. Pressed Ctrl + U (shortcut for "Go to Top") and observed the cursor moved to the first line and the first line got revealed in the editor.
  12. Pressed Ctrl + Shift + ] (shortcut for "Last Page") and observed the last page got loaded and the cursor moved to the last line.
  13. Pressed Ctrl + Shift + [ (shortcut for "First Page") and observed the first page got loaded and the cursor moved to the first line.
  14. Selected LogEventNum debug input box so that the editor was no longer focused. Pressed ` and observed the editor regained focus.

@junhaoliao junhaoliao changed the title Add MonacoInstance. new-log-viewer: Integrate Monaco Editor for enhanced log viewing. Aug 17, 2024
const editor = monaco.editor.create(
editorContainer,
{
// TODO: add custom observer debounce automatic layout
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be done in a follow up PR.

{line}
</p>
))}
<div style={{flexDirection: "column", flexGrow: 1}}>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flex approach has proven to be somewhat inefficient in laying out the editor. In a future PR where the other UI elements (sidebar, menu bar and status bar) are added, an absolute frame with definite top and bottom styles will be used for the editor instance container instead.

@junhaoliao junhaoliao marked this pull request as ready for review August 17, 2024 08:03
@junhaoliao junhaoliao removed the request for review from kirkrodrigues August 17, 2024 08:07
* @param editor
* @param onCursorExplicitPosChange
*/
const setupCursorExplicitPosChangeAction = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about setupCursorExplicitPosChangeCallback (or Handler)? The term "action" is a bit overused, so I think limiting it to things related to ActionType would be clearer.

Comment on lines 68 to 69
* @param touch1
* @param touch2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

touch0 and touch1 for consistency?

};

/**
* Initializes a Monaco Editor instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Initializes a Monaco Editor instance.
* Creates and initializes a Monaco Editor instance.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review 1/x

* @param handlers
* @return The initialized editor instance.
*/
const initMonacoEditor = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about createMonacoEditor?

new-log-viewer/src/components/Editor/index.tsx Outdated Show resolved Hide resolved
new-log-viewer/src/components/Layout.tsx Outdated Show resolved Hide resolved
new-log-viewer/src/utils/actions.ts Outdated Show resolved Hide resolved
new-log-viewer/src/utils/actions.ts Outdated Show resolved Hide resolved
new-log-viewer/src/utils/actions.ts Outdated Show resolved Hide resolved
/* eslint-disable sort-keys */
const EDITOR_ACTIONS : ActionType[] = [
{
action: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read through all the code, but so far I'm not sure why we allow action to be null (in the same vein as why we can't handle Backquote with setupCustomActions).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we can't handle Backquote with setupCustomActions

When the monaco container is not focused, any keydown events are not picked up by Monaco and therefore no Monaco customer actions can be triggered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. So then do we need the action for the backquote?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline - to be used in rendering a help menu for shortcuts. It might be better to document why we have null values here though.

@junhaoliao junhaoliao deleted the monaco-instance branch August 24, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants