search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.67k stars 8.23k forks source link

[ES|QL] Multi-line Comments is not colored correctly #197250

Open ninoslavmiskovic opened 1 month ago

ninoslavmiskovic commented 1 month ago

The ES|QL editor (as seen in version 9.0.0) does not properly display multi-line comments. The issue involves comment blocks (/ ... /) where the content is not rendered clearly, possibly leading to formatting or readability issues.

Steps to Reproduce:

1.  Open Discover in ES|QL.
2.  Enter the following query into the ES|QL editor:
FROM kibana_sample_data_logs
KEEP @timestamp, event.dataset, agent

// This is a single-line comment

/*
This is a multi-line comment.
Here you can write a lot of "meow"
and still be in one comment block.
Meow meow meow!
*/
  1. See the editor’s handling of the multi-line comment.

Expected Behavior: The multi-line comment should be fully visible and displayed in a block format, without any truncation or rendering issues.

Actual Behavior: As seen in the attached screenshot, the multi-line comment appears misaligned or is not fully rendered, making it difficult to read the entire comment block.

Kibana version: 9.0.0

Image

Image

elasticmachine commented 1 month ago

Pinging @elastic/kibana-esql (Team:ESQL)

drewdaemon commented 1 month ago

Looked into this a bit. Monaco gathers tokens line-by-line, it does not look across lines by default. In other words, when it gathers tokens for line 6, it has "forgotten" that it saw the start of a comment on line 5.

The way to make it remember things between lines is to record them in the state.

In this case, we should be recording that the tokenizer has seen an opening /* and no closing */ yet.

drewdaemon commented 1 month ago

Maybe blocked on https://github.com/elastic/elasticsearch/issues/115483

drewdaemon commented 1 month ago

I have a working PoC: https://github.com/elastic/kibana/pull/197545. But it currently relies on changes to the ANTLR lexer which would need to be submitted to the Elasticsearch repo. This seems like the correct way to do it. However, we might be able to hack something together on our side as well. I have added an agenda item to dev sync tomorrow to discuss.

drewdaemon commented 1 month ago

We had a good discussion. @vadimkibana pointed out that it would be nice to have a more generic solution than I currently have in https://github.com/elastic/kibana/pull/197545 because right now highlighting can be broken for any lexical token that spans multiple lines, not just multi-line comments.

We discussed the running the lexer on the entire query while still providing tokens to Monaco on a line-by-line basis. This would remove the need to improve lexing for incomplete queries. We will do some investigation at some point to see if this is a possibility.