-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Fixed rendered template values not displaying with proper code format… #53657
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
base: main
Are you sure you want to change the base?
Conversation
…ting and syntax highlighting in the Task Instance UI
@pierrejeambrun, @jscheffl thoughts on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading code I really like the lean approach. Some matching nit from my side to make it a bit more safe.
Would like to have an UI expert second opinion though.
SyntaxHighlighter.registerLanguage("sql", sql); | ||
SyntaxHighlighter.registerLanguage("bash", bash); | ||
|
||
const detectLanguage = (value: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you separate this function out into a separate TS module file?
} | ||
|
||
// Try to detect YAML (basic heuristics) | ||
if (trimmed.includes(":") && (trimmed.includes("\n") || trimmed.includes("- "))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detection safety:
- If a colon is matched it should be followed by a newline (
":\n"
) - Should we not rather use a lib like https://www.npmjs.com/package/yaml to test parsing after some basic heuristics before making a decision (similar to JSON)?
// Try to detect SQL (basic heuristics) | ||
const sqlKeywords = /\b(?:select|insert|update|delete|create|alter|drop|from|where|join)\b/iu; | ||
|
||
if (sqlKeywords.test(trimmed)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it might be better to match more than a single keyword, as SELECT
will in almost all cases will be followed by a FROM
and otherwise DROP
will also usually be followed by a TABLE
.
This PR attempts to fix #50324 , #49064
The issue was caused by double JSON serialization in the frontend:
Core Fixes
Syntax Highlighting Enhancement