Skip to content

perf: Defer autoUpdate until menu is open to fix slow profile icon rendering#1450

Open
jjmata wants to merge 1 commit intomainfrom
claude/fix-profile-icon-rendering-sUdLI
Open

perf: Defer autoUpdate until menu is open to fix slow profile icon rendering#1450
jjmata wants to merge 1 commit intomainfrom
claude/fix-profile-icon-rendering-sUdLI

Conversation

@jjmata
Copy link
Copy Markdown
Collaborator

@jjmata jjmata commented Apr 12, 2026

The autoUpdate from @floating-ui/dom was being started in connect(), causing immediate layout/size calculations for every menu instance on page load even while closed. Moved autoUpdate to only run when the menu is open, and stop when closed/toggled off.

https://claude.ai/code/session_01FkTsyLvEod4qN6T5dNidkL

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed menu position tracking to activate only when the menu is open and deactivate when closed, improving performance and ensuring consistent behavior.

…ndering

autoUpdate from @floating-ui/dom was being started in connect(), causing
immediate layout/size calculations for every menu instance on page load
even while closed. Moved autoUpdate to only run when the menu is open,
and stop when closed/toggled off.

https://claude.ai/code/session_01FkTsyLvEod4qN6T5dNidkL
@jjmata jjmata self-assigned this Apr 12, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: def5d7d0-9cbc-4dfd-a782-b5aa207d4719

📥 Commits

Reviewing files that changed from the base of the PR and between c57c08c and 96b981c.

📒 Files selected for processing (1)
  • app/components/DS/menu_controller.js

📝 Walkthrough

Walkthrough

Modified lifecycle management for position auto-updates in the menu controller. Auto-updates no longer start unconditionally during connection; instead, they are conditionally managed in the toggle and close methods based on menu visibility state.

Changes

Cohort / File(s) Summary
Menu Controller Lifecycle Management
app/components/DS/menu_controller.js
Refactored auto-update lifecycle: removed automatic start from connect(), added conditional startAutoUpdate()/stopAutoUpdate() calls in toggle() based on menu visibility, and ensured close() stops auto-updates.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A hop, a toggle, a menu so bright,
Auto-updates dance when opened just right,
Close with a pause, the cycle winds down,
Lifecycle ballet in our UI town!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: deferring autoUpdate until menu is open to fix slow profile icon rendering. It directly matches the changeset's objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-profile-icon-rendering-sUdLI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sokie
Copy link
Copy Markdown
Collaborator

sokie commented Apr 12, 2026

passed this to claude with the HAR file from the main dashboard when I ran the tests/benchmarks:
in terms of the dashboard performance picture, the HAR showed only 2 DS--menu instances on the page.
So this saves 2 idle autoUpdate listeners. It's a correct fix but a minor one.
One thing to flag: if showValue is ever true (menu starts open), the PR won't start autoUpdate on connect. The current code does. Easy to miss since the default is false, but worth a guard.

Performance impact:

  • Dashboard: minor (2 menu instances, saves 2 idle ResizeObservers)
  • Pages with many menus (transactions list, activity views): more meaningful
  • The real win is eliminating idle computePosition() calls that trigger forced DOM layout reads

One suggestion (not blocking): if showValue is ever set to true in the future (menu starts open), connect() should handle it:

connect() {                                                                                                                                                                 
    this.show = this.showValue;                                                                                                                                             
    this.boundUpdate = this.update.bind(this);                                                                                                                              
    this.addEventListeners();                                                                                                                                               
    if (this.show) {         
        this.startAutoUpdate();                                                                                                                                             
    }                                                                                                                                                                       
}

Currently no callers set showValue: true, so this is defensive, not urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants