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

[Console Monaco migration] Implement history #183181

Merged
merged 35 commits into from
May 22, 2024

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented May 10, 2024

Summary

Fixes #182948

This PR migrates history component from ace to monaco and re-implements the logic to insert the saved requests into the monaco editor.

To test:

  1. click the "History" tab and check that the component is using Monaco to display requests
  2. Check that "Clear history" button works
  3. Check that a request from history can be inserted into the editor
  • when the cursor is on the 1st line of the request, the history request is inserted before the request in the editor
  • if the cursor is not on the 1st line of the request, the history request is inserted after the request in the editor

Screen recording

Screen.Recording.2024-05-15.at.18.37.43.mov

@yuliacech yuliacech added Feature:Console Dev Tools Console Feature Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels May 10, 2024
@yuliacech
Copy link
Contributor Author

/ci

@yuliacech
Copy link
Contributor Author

/ci

@yuliacech
Copy link
Contributor Author

/ci

@yuliacech
Copy link
Contributor Author

/ci

@yuliacech yuliacech marked this pull request as ready for review May 15, 2024 16:49
@yuliacech yuliacech requested a review from a team as a code owner May 15, 2024 16:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

@yuliacech yuliacech added release_note:skip Skip the PR/issue when compiling release notes v8.15.0 labels May 15, 2024
@ElenaStoeva ElenaStoeva self-requested a review May 16, 2024 16:00
Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @yuliacech! Tested locally and overall the History functionality works well! I only noticed one issue that occurs when we have two consequent requests and we try to insert one from history before the second request:

Screen.Recording.2024-05-16.at.17.10.26.mov

It seems that the line above the first line of the selected request is separated from the first request and the request from history is put right above it. 🤔

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @ElenaStoeva!
Great catch with this bug, I fixed the implementation and added some unit tests for that function (a lot of mocking necessary, but still better than nothing until we can do e2e testing).

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Amazing job @yuliacech! 🎉 Thanks for fixing the bug and adding unit tests (I will try to use the same mocking technique to add unit tests for the keyboard commands PR)!

After some further testing, I noticed something small, which is not really a bug but just a small difference from the behavior in Ace. Let me know what you think.

Everything else LGTM, so approving the PR to unblock.

prefix = '\n';
}
} else {
// if not inside a request, insert the request at the cursor position
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the request from history will always end up at column 1 unless it is not inside a request - then it might end up in the middle of the line if the cursor is there. In Ace editor, it always ends up at column 1. Should we do the same here for consistency?

Suggested change
// if not inside a request, insert the request at the cursor position
// the cursor is not inside a request
if (!position) {
// if no cursor position, insert at the beginning of the first line
position = { lineNumber: 1, column: 1 };
} else {
// insert the request at the beginning of the line of the cursor position
position = { lineNumber: position.lineNumber, column: 1 };
}

In Ace editor:

Screen.Recording.2024-05-17.at.17.00.22.mov

In this PR:

Screen.Recording.2024-05-17.at.16.58.34.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, @ElenaStoeva! Another great catch, fixed in 67968c6

@yuliacech yuliacech enabled auto-merge (squash) May 21, 2024 16:32
@yuliacech yuliacech disabled auto-merge May 22, 2024 09:36
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
console 257 259 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
console 452.3KB 454.9KB +2.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yuliacech yuliacech merged commit 9194c88 into elastic:main May 22, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console Monaco migration] Fix editor history panel
5 participants