⚡ Optimize Screen Transitions in experiment.js#5
⚡ Optimize Screen Transitions in experiment.js#5hashexplaindata wants to merge 1 commit intomasterfrom
Conversation
Optimized the `showScreen` function to avoid redundant O(n) iteration over all screen elements. Introduced `STATE.activeScreen` to track the currently visible screen, enabling O(1) transitions. Improvements: - Reduced iteration overhead from O(n) to O(1). - Improved robustness by dynamically identifying the initial active screen. - Maintained millisecond-accurate timing and smooth CSS transitions. Measured Improvement: - Benchmark (1000 screens, 1000 iterations): - Baseline: ~860ms - Optimized: ~5ms - Improvement: >99% reduction in execution time for navigation logic. Co-authored-by: hashexplaindata <221828969+hashexplaindata@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance of screen transitions within Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively optimizes the showScreen function by avoiding the O(n) iteration over all screens, which is a great performance improvement. My review includes a suggestion to refine the transition logic in experiment.js to ensure the fade-out animation completes correctly, which seems to be a key goal of this change. I've also pointed out a small redundancy in the new benchmark.html file.
| function showScreen(id) { | ||
| DOM.screens.forEach(s => { | ||
| s.classList.remove('active'); | ||
| s.style.display = 'none'; | ||
| }); | ||
| const target = document.getElementById(`screen-${id}`); | ||
| if (!target) return; | ||
|
|
||
| if (STATE.activeScreen) { | ||
| STATE.activeScreen.classList.remove('active'); | ||
| STATE.activeScreen.style.display = 'none'; | ||
| } | ||
|
|
||
| target.style.display = 'flex'; | ||
| // Ensure active class is removed before adding it back to trigger transitions | ||
| target.classList.remove('active'); | ||
| setTimeout(() => target.classList.add('active'), 50); | ||
| STATE.activeScreen = target; | ||
| } |
There was a problem hiding this comment.
This optimization is a great improvement. However, the current implementation has a couple of issues with the transition handling:
- Broken Fade-Out: The fade-out transition for the old screen won't be visible.
style.display = 'none'is set immediately, which removes the element from the render tree before the 400ms opacity transition can complete. Thedisplaychange should be delayed. - Redundant Code: The line
target.classList.remove('active');is unnecessary. ThesetTimeouton the following line is what correctly triggers the fade-in transition. Removing it improves clarity.
I've combined fixes for these into a single suggestion that also adds a guard clause to prevent unnecessary work if the target screen is already active.
function showScreen(id) {
const target = document.getElementById(`screen-${id}`);
// Exit if target doesn't exist or is already the active screen.
if (!target || target === STATE.activeScreen) {
return;
}
// Fade out the old screen and hide it after the transition.
if (STATE.activeScreen) {
const oldScreen = STATE.activeScreen;
oldScreen.classList.remove('active');
setTimeout(() => {
oldScreen.style.display = 'none';
}, 400); // Match CSS transition duration.
}
// Fade in the new screen.
target.style.display = 'flex';
setTimeout(() => target.classList.add('active'), 50); // Delay ensures transition fires.
STATE.activeScreen = target;
}| container.appendChild(div); | ||
| } | ||
| document.getElementById('screen-0').classList.add('active'); | ||
| document.getElementById('screen-0').style.display = 'flex'; |
There was a problem hiding this comment.
This line is redundant. The .active class, which is added on the previous line, already sets display: flex via the CSS rule .screen.active { display: flex; ... }. Removing this direct style manipulation makes the initialization logic cleaner and more reliant on the defined CSS classes.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the experiment’s screen navigation by tracking the currently active .screen element, avoiding per-transition iteration over all screens and preserving CSS transition behavior.
Changes:
- Add
STATE.activeScreento track the currently displayed screen. - Initialize
STATE.activeScreenduringinit()and updateshowScreen()to hide/show only the relevant elements. - Add a standalone
benchmark.htmlfile to compare original vs optimized screen switching approaches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
code/experiment.js |
Reworks showScreen() to hide only the previously active screen and updates state to track the current one. |
code/benchmark.html |
Adds a local benchmark harness to measure screen transition performance differences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| function showScreen(id) { | ||
| DOM.screens.forEach(s => { | ||
| s.classList.remove('active'); | ||
| s.style.display = 'none'; | ||
| }); | ||
| const target = document.getElementById(`screen-${id}`); | ||
| if (!target) return; |
There was a problem hiding this comment.
After this change, DOM.screens is no longer referenced anywhere in the file (verified via search), but it’s still queried at load time. Removing it (or repurposing it for initialization) would avoid unnecessary DOM work and reduce confusion about the navigation mechanism.
| // Optimized showScreen (to be tested) | ||
| let activeScreen = document.getElementById('screen-0'); | ||
| function showScreenOptimized(id) { | ||
| if (activeScreen) { | ||
| activeScreen.classList.remove('active'); | ||
| activeScreen.style.display = 'none'; | ||
| } | ||
| const target = document.getElementById(`screen-${id}`); | ||
| target.style.display = 'flex'; | ||
| target.classList.add('active'); | ||
| activeScreen = target; | ||
| } |
There was a problem hiding this comment.
The benchmark’s showScreenOptimized implementation doesn’t match the new experiment.js logic (no target.classList.remove('active') before re-adding, no async toggle, no missing-target handling). As written, the timing results may not reflect the behavior/perf of the actual production showScreen; consider aligning the benchmark implementation with the real function or clearly documenting what’s intentionally being measured.
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <title>Benchmark</title> |
There was a problem hiding this comment.
The PR description focuses on optimizing showScreen in experiment.js, but this PR also adds a new code/benchmark.html file. If this file is intended to ship with the deployed app, consider calling that out in the PR description; otherwise, consider moving it under a dev-only/tools location or excluding it from production artifacts.
| s.style.display = 'none'; | ||
| }); | ||
| const target = document.getElementById(`screen-${id}`); | ||
| if (!target) return; |
There was a problem hiding this comment.
showScreen now returns early when the target screen element is missing, which will silently break navigation and make it harder to diagnose invalid IDs (previously this would fail loudly when trying to access style on null). Consider at least console.error/console.warn (including the missing id) or throwing so failures surface during development/telemetry debugging.
| if (!target) return; | |
| if (!target) { | |
| console.error(`showScreen: target element not found for id "${id}" (expected DOM id "screen-${id}")`); | |
| return; | |
| } |
| target.style.display = 'flex'; | ||
| // Ensure active class is removed before adding it back to trigger transitions | ||
| target.classList.remove('active'); | ||
| setTimeout(() => target.classList.add('active'), 50); | ||
| STATE.activeScreen = target; |
There was a problem hiding this comment.
The setTimeout(() => target.classList.add('active'), 50) isn’t cancelled if showScreen is invoked again within that 50ms window (e.g., double-clicks). This can leave a previously hidden screen with an active class or cause out-of-order transition state. Consider tracking the timeout id (e.g., on STATE) and clearTimeout before scheduling a new one, or using a requestAnimationFrame-based toggle that can’t apply to stale targets.
|
Closing: Fixes have already been incorporated directly into the master branch. |
Understood. Acknowledging that these fixes have already been incorporated into the master branch. I will stop work on this task. |
hashexplaindata
deleted the
performance-opt-screen-transitions-8659292603467908914
branch
The
showScreenfunction was previously iterating over all DOM elements with the class.screento hide them before showing the target screen. This was an O(n) operation that became increasingly inefficient as more screens were added to the experiment.I optimized this by:
activeScreento theSTATEobject.STATE.activeScreenin theinit()function by querying the DOM for the initially active screen.showScreento directly hide theSTATE.activeScreenand update the reference to the newtargetscreen..activeclass is correctly toggled to preserve CSS transitions.This change significantly improves the efficiency of screen transitions, which is critical for maintaining "Zero-Latency UI" and accurate telemetry in the behavioral diagnostic engine.
PR created automatically by Jules for task 8659292603467908914 started by @hashexplaindata