codeant-ai-for-open-source[bot] commented on code in PR #41697: URL: https://github.com/apache/superset/pull/41697#discussion_r3531109771
########## superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/emojiWidthPatch.ts: ########## @@ -0,0 +1,267 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Ace positions the caret on a monospace grid (`column × charWidth`) and + * renders anything it classifies as "full width" inside a forced + * `2 × charWidth` inline-block (`.ace_cjk`), so the grid model and the pixels + * agree. Its width tables only cover East-Asian ranges, though: + * + * - Astral emoji (💡, surrogate pairs) happen to work: the renderer's + * surrogate-pair branch forces the 2-cell box and the column counter + * counts the two code units. + * - BMP emoji with default emoji presentation (✨ U+2728, ⭐ U+2B50, …) fall + * through BOTH nets: they count as 1 column but render ~1.6 cells wide, so + * the caret drifts on any line containing one (issue #41664, upstream + * ajaxorg/ace#3404). + * - VS16 sequences (❤️ = U+2764 U+FE0F) render as one color glyph while the + * two code units count as two ordinary columns, without the forced box. + * + * The patch extends the exact mechanism Ace already uses — count 2 columns + * and force the 2-cell box — to emoji-presentation codepoints and VS16 + * sequences, by overriding the two `EditSession` width methods (verbatim + * copies of the ace-builds internals plus one branch; the originals call a + * module-private `isFullWidth`, so they cannot be extended in place) and + * wrapping the text layer's `$renderToken` to emit the forced-width span for + * emoji runs before delegating everything else to the original. + * + * Deliberately out of scope: ZWJ sequences (family emoji) and flag pairs + * render as one glyph but count per component; handling them needs grapheme + * segmentation across every Ace layer, and they were equally misaligned + * before this patch. + */ + +const VS16 = 0xfe0f; + +// Emoji_Presentation covers exactly the codepoints browsers render as color +// emoji without a variation selector. Lone surrogates never match it (the +// astral path is already consistent), so only BMP emoji change behavior. +const EMOJI_PRESENTATION_RE = /\p{Emoji_Presentation}/u; +// One renderable emoji cluster: an emoji-presentation codepoint with an +// optional (redundant) VS16, or a text-presentation emoji forced to color +// presentation by VS16. +const EMOJI_CLUSTER_SOURCE = + '(?:\\p{Emoji_Presentation}\\uFE0F?|\\p{Emoji}\\uFE0F)'; +const EMOJI_RUN_TEST_RE = new RegExp(`${EMOJI_CLUSTER_SOURCE}`, 'u'); +const EMOJI_RUN_SPLIT_RE = new RegExp(`(${EMOJI_CLUSTER_SOURCE}+)`, 'gu'); +const EMOJI_CLUSTER_RE = new RegExp(EMOJI_CLUSTER_SOURCE, 'gu'); + +// Memoized per-code-unit classification; $getStringScreenWidth runs per +// character per rendered line, so the regexes must not run every time. +const EMOJI_BASE_RE = /\p{Emoji}/u; +const emojiBaseCache = new Map<number, boolean>(); +function isEmojiBase(code: number): boolean { + let result = emojiBaseCache.get(code); + if (result === undefined) { + result = EMOJI_BASE_RE.test(String.fromCharCode(code)); + emojiBaseCache.set(code, result); + } + return result; +} + +const bmpEmojiCache = new Map<number, boolean>(); +export function isBmpEmojiPresentation(code: number): boolean { + if (code < 0x2000 || code > 0xffff || (code >= 0xd800 && code <= 0xdfff)) { + return false; + } + let result = bmpEmojiCache.get(code); + if (result === undefined) { + result = EMOJI_PRESENTATION_RE.test(String.fromCharCode(code)); + bmpEmojiCache.set(code, result); + } + return result; +} + +/** + * Screen-column contribution of one code unit under the emoji rules, or null + * when Ace's default handling should apply. Emoji-presentation chars are 2 + * columns; VS16 tops its base up to 2 in total (so ❤️ = 1 + 1 and a + * redundant ✨️ = 2 + 0). + */ +export function emojiWidth(code: number, prevCode: number): number | null { + if (isBmpEmojiPresentation(code)) { + return 2; + } + if (code === VS16) { + return isBmpEmojiPresentation(prevCode) ? 0 : 1; + } + return null; +} + +interface PatchableEditSessionClass { + prototype: { + isFullWidth: (c: number) => boolean; + getScreenTabSize: (screenColumn: number) => number; + $getStringScreenWidth: ( + str: string, + maxScreenColumn?: number, + screenColumn?: number, + ) => [number, number]; + $getDisplayTokens: (str: string, offset: number) => number[]; + }; +} + +interface TextLayerInstance { + config: { characterWidth: number }; + dom: { createElement: (tag: string) => HTMLElement }; +} + +type RenderToken = ( + this: TextLayerInstance, + parent: HTMLElement, + screenColumn: number, + token: { type: string; value: string }, + value: string, +) => number; + +interface PatchableTextLayerClass { + prototype: { $renderToken: RenderToken }; +} + +let patched = false; + +export function patchAceEmojiWidths( + EditSession: PatchableEditSessionClass, + TextLayer: PatchableTextLayerClass, +): void { + if (patched) { + return; + } + patched = true; + + const sessionProto = EditSession.prototype; + const { isFullWidth } = sessionProto; + + // Mirrors EditSession.prototype.$getStringScreenWidth from ace-builds + // (src-noconflict/ace.js), adding the emojiWidth branch. + sessionProto.$getStringScreenWidth = function $getStringScreenWidth( + this: PatchableEditSessionClass['prototype'], + str: string, + maxScreenColumn?: number, + screenColumn?: number, + ): [number, number] { + if (maxScreenColumn === 0) { + return [0, 0]; + } + const max = maxScreenColumn ?? Infinity; + let screen = screenColumn || 0; + let column; + for (column = 0; column < str.length; column += 1) { + const c = str.charCodeAt(column); + const emoji = emojiWidth(c, column > 0 ? str.charCodeAt(column - 1) : 0); + if (c === 9) { + screen += this.getScreenTabSize(screen); + } else if (emoji !== null) { + screen += emoji; + } else if (c >= 0x1100 && isFullWidth(c)) { + screen += 2; + } else { + screen += 1; + } + if (screen > max) { + break; + } + } + return [screen, column]; + }; + + // Display-token codes from ace's edit_session module. + const CHAR = 1; + const CHAR_EXT = 2; + const PUNCTUATION = 9; + const SPACE = 10; + const TAB = 11; + const TAB_SPACE = 12; + + // Mirrors EditSession.prototype.$getDisplayTokens, adding the emoji branch. + // One entry per code unit is preserved (VS16 pushes a lone CHAR_EXT) so the + // wrap-split offset math stays aligned with document columns. + sessionProto.$getDisplayTokens = function $getDisplayTokens( + this: PatchableEditSessionClass['prototype'], + str: string, + offset: number, + ): number[] { + const arr: number[] = []; + let tabSize: number; + for (let i = 0; i < str.length; i += 1) { + const c = str.charCodeAt(i); + const emoji = emojiWidth(c, i > 0 ? str.charCodeAt(i - 1) : 0); + if (c === 9) { + tabSize = this.getScreenTabSize(arr.length + offset); + arr.push(TAB); + for (let n = 1; n < tabSize; n += 1) { + arr.push(TAB_SPACE); + } + } else if (emoji === 2) { + arr.push(CHAR, CHAR_EXT); + } else if (emoji === 0) { + arr.push(CHAR_EXT); + } else if (c === VS16 && i > 0 && isEmojiBase(str.charCodeAt(i - 1))) { + // VS16 after a text-presentation emoji base (❤️): keep the pair + // atomic for wrap splitting; its width contribution stays 1. + arr.push(CHAR_EXT); + } else if (c === 32) { + arr.push(SPACE); + } else if ((c > 39 && c < 48) || (c > 57 && c < 64)) { + arr.push(PUNCTUATION); + } else if (c >= 0x1100 && isFullWidth(c)) { + arr.push(CHAR, CHAR_EXT); + } else { + arr.push(CHAR); + } + } + return arr; + }; + + // Pre-split token text on emoji runs: emoji clusters get the same forced + // 2 × charWidth `.ace_cjk` box Ace gives CJK (so rendered geometry matches + // the 2-column model above); everything else delegates to the original. + const origRenderToken = TextLayer.prototype.$renderToken; + TextLayer.prototype.$renderToken = function $renderToken( + this: TextLayerInstance, + parent: HTMLElement, + screenColumn: number, + token: { type: string; value: string }, + value: string, + ): number { + if (!EMOJI_RUN_TEST_RE.test(value)) { + return origRenderToken.call(this, parent, screenColumn, token, value); + } + let column = screenColumn; + value.split(EMOJI_RUN_SPLIT_RE).forEach((part, index) => { + if (!part) { + return; + } + const isEmojiRun = index % 2 === 1; + if (isEmojiRun) { + (part.match(EMOJI_CLUSTER_RE) ?? [part]).forEach(cluster => { + const span = this.dom.createElement('span'); + span.style.width = `${this.config.characterWidth * 2}px`; + span.className = 'ace_cjk'; + span.textContent = cluster; + parent.appendChild(span); + column += 2; + }); + } else { + column = origRenderToken.call(this, parent, column, token, part); Review Comment: **Suggestion:** The custom emoji rendering path bypasses Ace’s normal token rendering and only assigns `ace_cjk`, so emoji segments no longer inherit the token-specific syntax classes (like keyword/string/comment classes). This causes visible syntax-highlighting regressions whenever a token contains emoji. Preserve the original token class context when rendering emoji spans (for example by wrapping emoji output in the same token-class container Ace uses). [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ SQL Lab tokens with emoji show inconsistent highlighting. - ⚠️ Dashboard Markdown/CSS editors miscolor emoji in tokens. - ⚠️ Syntax themes misapply styles to emoji spans. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the AsyncAceEditor initialization code at `superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx` (lines 24-30 in the loaded snippet) and note that `patchAceEmojiWidths` is called once globally: - `patchAceEmojiWidths(acequire('ace/edit_session').EditSession, acequire('ace/layer/text').Text);` This overrides `TextLayer.prototype.$renderToken` for all Ace editors used in Superset (SQL Lab, dashboard Markdown, CSS editors, etc.). 2. Inspect the emoji width patch implementation at `superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/emojiWidthPatch.ts` around lines 232-267 (verified via BulkRead). The override of `TextLayer.prototype.$renderToken` is: - At lines 232-235: `const origRenderToken = TextLayer.prototype.$renderToken;` - At lines 236-242: new `$renderToken` is defined, and when `EMOJI_RUN_TEST_RE.test(value)` is true, execution enters the emoji-specific branch. 3. Follow the emoji branch in the same file at lines 252-262 (the `existing_code` snippet): - The code splits the token text with `value.split(EMOJI_RUN_SPLIT_RE)` and, for each emoji run (`isEmojiRun` true), creates a span: - `const span = this.dom.createElement('span');` - `span.style.width = \`${this.config.characterWidth * 2}px\`;` - `span.className = 'ace_cjk';` - `span.textContent = cluster;` - `parent.appendChild(span);` - Crucially, this span only receives the `ace_cjk` class and is appended directly to `parent`, without any of the token-type-specific classes derived from `token.type` (e.g. `.ace_keyword`, `.ace_string`, `.ace_comment`) that Ace normally applies in its original `origRenderToken` implementation. 4. Observe the practical effect in a real feature that uses AsyncAceEditor. For example, the SQL Lab editor uses `FullSQLEditor` from `@superset-ui/core/components/AsyncAceEditor` (confirmed by `superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx` lines 57-75, which mock that export), so in production the actual Ace-based editor is active: - Start Superset with this PR code, open SQL Lab, and type a token containing emoji, e.g. a SQL comment `-- ✨ important` or string literal `'✨ important'`. - In the browser devtools, inspect the rendered line: non-emoji parts of the token are in spans created by `origRenderToken` with classes like `.ace_comment` or `.ace_string`, while the emoji cluster is in a separate `<span class="ace_cjk">✨</span>` appended directly under the line container. - Because the emoji span lacks the token-type class, it no longer matches the theme’s syntax-highlighting rules for that token type (color, font style). The emoji thus renders with default editor styling instead of the keyword/string/comment color, causing visible syntax-highlighting inconsistency whenever a token includes emoji. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b49c5db045647229bc60e92028612e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7b49c5db045647229bc60e92028612e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/emojiWidthPatch.ts **Line:** 252:262 **Comment:** *Logic Error: The custom emoji rendering path bypasses Ace’s normal token rendering and only assigns `ace_cjk`, so emoji segments no longer inherit the token-specific syntax classes (like keyword/string/comment classes). This causes visible syntax-highlighting regressions whenever a token contains emoji. Preserve the original token class context when rendering emoji spans (for example by wrapping emoji output in the same token-class container Ace uses). Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41697&comment_hash=a77a727ef15578727803ddf2db80e9106e811700f4094f0eca908151700733e4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41697&comment_hash=a77a727ef15578727803ddf2db80e9106e811700f4094f0eca908151700733e4&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
