EnxDev commented on PR #41697:
URL: https://github.com/apache/superset/pull/41697#issuecomment-4874605876
## EnxDev's Review Agent โ apache/superset#41697 ยท HEAD 6b7dca5
comment โ the change is safe and CI-green, but the vendored Ace source
contradicts its root-cause premise: this very likely does not fix #41664, and
`Fixes #41664` will auto-close the issue on merge.
### ๐ด Functional
-
**`superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:296`**
ยท _Medium_ โ The re-measure is very likely a no-op against the repro.
ace-builds `^1.41.0` (1.44.0 installed) already re-measures continuously:
`FontMetrics` attaches a `ResizeObserver` to its measure node (`USE_OBSERVER =
typeof ResizeObserver == "function"` โ `$addObserver` โ `checkForSizeChanges`),
with ~500 ms idle polling as the no-ResizeObserver fallback โ so a late font
load self-corrects today on every modern browser, without this patch. A drift
that *persists* (the screencast in #41664) implies the measure node stably
renders different metrics than the text layer โ plausible since the #38928
`!important` font-family targets `.ace_content`/`.ace_text-layer` while the
measure node is a direct child of `.ace_editor` and uses the container font; in
that state `checkForSizeChanges()` sees "no change" and `updateFontSize()` does
nothing. Impact: merging auto-closes the iss
ue while users still see the drift. Could we get the visual confirm on the
repro (as the PR body already requests) before merge โ and if it doesn't
reproduce-fix, compare computed `font-family`/`letter-spacing` of the
FontMetrics measure node vs `.ace_text-layer` on the repro instead?
**regression test:** not possible headlessly โ gate the merge on the manual
repro check.
### ๐ก Should-fix
-
**`superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:285`**
โ the comment "Ace measures glyph width once, when the editor is constructed"
is contradicted by the vendored source (ResizeObserver + idle polling
re-measure continuously) โ correct it so the next reader doesn't inherit a
wrong model; same claim in the PR body.
### ๐ต Nits
-
`superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:302`
โ the post-`fonts.ready` remeasure can fire after unmount on a destroyed
editor; harmless today only because `.catch(() => {})` swallows the throw โ a
disposal guard would make it explicit.
-
`superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/AsyncAceEditor.test.tsx:87`
โ the spy is installed inside the consumer `onLoad`, so it can only observe
the post-`fonts.ready` call; spying on
`VirtualRenderer.prototype.updateFontSize` before render would also cover the
immediate re-measure.
### ๐ Praise
- `AsyncAceEditor.test.tsx:76` โ careful `document.fonts` descriptor
save/restore, and destructuring `onLoad` out of props so `{...props}` can't
clobber the wrapper is exactly right.
<!-- enxdev-review-agent:6b7dca5 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 6b7dca5._
--
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]
