Conversation
alvinttang
left a comment
There was a problem hiding this comment.
Code Review
Bug: gradioApp() called at module load time — crashes before Gradio initializes
File: javascript/aspectRatioOverlay.js, lines 4-10
const gradio = gradioApp(); // cache gradioApp reference
let arPreviewRect = gradio.querySelector('#imageARPreview');
if (!arPreviewRect) {
arPreviewRect = document.createElement('div');
arPreviewRect.id = "imageARPreview";
gradio.appendChild(arPreviewRect);
}This code runs at module parse time (top-level script execution). If the script is loaded before Gradio has fully initialized the DOM, gradioApp() will return null or a stub element, causing gradio.querySelector(...) to throw a TypeError. The original code called gradioApp() inside dimensionChange() (which runs on user input, well after initialization) and inside onAfterUiUpdate() (which runs after UI is ready) — both safe timing points.
Caching gradioApp() at the top level defeats the lazy-initialization pattern that the rest of the codebase relies on.
Bug: requestAnimationFrame + setTimeout interaction causes stale closure over arFrameTimeout
if (arFrameTimeout) cancelAnimationFrame(arFrameTimeout);
arFrameTimeout = requestAnimationFrame(() => {
setTimeout(() => {
arPreviewRect.style.display = 'none';
}, 2000);
});The original code used clearTimeout(arFrameTimeout) with setTimeout — both operating on the same timer ID space. The new code stores a requestAnimationFrame ID in arFrameTimeout and cancels it with cancelAnimationFrame. However, once the rAF callback fires (typically within ~16ms), the inner setTimeout is scheduled and cannot be cancelled — cancelAnimationFrame on subsequent calls won't cancel the already-fired rAF or its inner timeout.
If dimensionChange fires rapidly (e.g., dragging a slider), you get N setTimeout callbacks queued, each hiding the preview after 2 seconds. The original clearTimeout approach correctly cancelled the previous timeout before setting a new one. The rAF wrapper here adds no benefit (the operation is not animation-related) and breaks the cancellation semantics.
Suggested fix: keep setTimeout / clearTimeout as in the original, or store the inner setTimeout ID separately and clear it.
Nit: zoom.js rename zoom to zoomLevel is a breaking change if extensions read elemData
File: extensions-builtin/canvas-zoom-and-pan/javascript/zoom.js, L289
The property name change from zoom to zoomLevel in elemData[elemId] will break any extension or user script that reads elemData[elemId].zoom. If this data structure is part of a public/semi-public API, this needs a deprecation path or at least mention in release notes.
Refactored the dimensionChange logic and AR preview handling for better performance and readability.
The code now caches repeated DOM queries like gradioApp() and the AR preview div to avoid unnecessary lookups on every input event.
Type conversions were simplified, and redundant calculations were reduced for efficiency.
The AR preview rendering now uses requestAnimationFrame instead of setTimeout for smoother updates, ensuring better user experience.
Event listeners for width and height inputs are added only once, and input value handling is streamlined for clarity.
Overall, this reduces memory usage and improves responsiveness when updating image dimensions.
Additional optimizations include combining repetitive logic for tab selection, scaling, and centering calculations,
making the code easier to maintain and extend in the future. This commit keeps all existing functionality intact while improving performance and maintainability.