fix(ds): keep DS::Menu/Popover panels anchored across Turbo morphs#2812
fix(ds): keep DS::Menu/Popover panels anchored across Turbo morphs#2812gariasf wants to merge 1 commit into
Conversation
The app refreshes pages via Turbo morph (`turbo_refreshes_with method: :morph`), and same-page account actions (disable, exclude, set-default, etc.) trigger one. Two bugs in the shared floating-ui controllers surface as a result: - The panel's `position: fixed` only ever existed as a JS-applied inline style. Idiomorph resets every menu/popover's `style` attribute to match the server-rendered markup (which has none), silently stripping `position: fixed` from every panel on the page. The next dropdown/ popover opened before floating-ui's async recompute lands briefly renders in normal flex flow, shoving its own trigger sideways and making computePosition anchor to that phantom position instead of the real button. Fix: make `position: fixed` part of the static markup so it can never be stripped. - `this.show` was a plain instance property. Because the morph preserves the Stimulus controller in place (stable-id turbo frame), `this.show` doesn't reset when idiomorph re-closes the content element, so it can desync from the DOM and swallow the next click. Fix: derive `show` from the content element's own class instead of tracking it separately. Reproduced and verified against the actual Turbo/Stimulus/floating-ui pipeline in an isolated harness before and after the fix.
📝 WalkthroughWalkthroughMenu and popover content containers now use fixed positioning. Their controllers derive visibility from the DOM’s ChangesDS overlay state and positioning
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/DS/menu_controller.js (1)
32-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for DOM-derived overlay state.
app/components/DS/menu_controller.js#L32-L40: test morphing an open menu back tohidden, then toggling it open again.app/components/DS/menu_controller.js#L115-L127: assert the reopened menu setsaria-expanded="true".app/components/DS/popover_controller.js#L33-L41: test the equivalent popover morph path.app/components/DS/popover_controller.js#L79-L90: assert the reopened popover setsaria-expanded="true".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/DS/menu_controller.js` around lines 32 - 40, Apply regression coverage at app/components/DS/menu_controller.js:32-40 and app/components/DS/menu_controller.js:115-127 by morphing an open menu to hidden, toggling it open again, and asserting aria-expanded="true"; add the equivalent popover coverage at app/components/DS/popover_controller.js:33-41 and app/components/DS/popover_controller.js:79-90, including the reopened aria-expanded assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/components/DS/menu_controller.js`:
- Around line 32-40: Apply regression coverage at
app/components/DS/menu_controller.js:32-40 and
app/components/DS/menu_controller.js:115-127 by morphing an open menu to hidden,
toggling it open again, and asserting aria-expanded="true"; add the equivalent
popover coverage at app/components/DS/popover_controller.js:33-41 and
app/components/DS/popover_controller.js:79-90, including the reopened
aria-expanded assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b50d66a-b0f3-449a-a55b-c3fd30adf683
📒 Files selected for processing (4)
app/components/DS/menu.html.erbapp/components/DS/menu_controller.jsapp/components/DS/popover.html.erbapp/components/DS/popover_controller.js
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Nice diagnosis — a Turbo morph resetting the server-rendered style attribute stripping the JS-applied position: fixed, combined with show being a plain instance property that can desync from the DOM after a morph (silently swallowing the next click), are both subtle bugs that would be easy to misdiagnose as "flaky floating-ui positioning."
The fixes address both root causes directly rather than patching symptoms:
- Moving
position: fixedinto the static class list means it survives morph's inline-style reset, whileleft/topstay JS-driven as before. - Deriving
showfromcontentTarget.classList.contains("hidden")instead of tracking separate state makes desync structurally impossible — it can't drift from the DOM because it is the DOM.
Applying the identical fix to DS::Popover since it mirrors DS::Menu's implementation is the right move rather than leaving a latent duplicate bug.
One soft suggestion: the verification here was a manual/isolated-harness reproduction against the real Turbo/Stimulus/floating-ui stack plus a live browser check — solid, but if there's a reasonably cheap way to script the "trigger a morph, then open a different row's dropdown" sequence as a system test, it'd guard against regression here specifically (this class of morph-timing bug is easy to reintroduce). Not a blocker given how thoroughly it was manually verified.
Generated by Claude Code
2 participants
Summary
turbo_refreshes_with method: :morph). Idiomorph resets everyDS::Menu/DS::Popovercontent panel'sstyleattribute back to the bare server-rendered value, silently stripping the JS-appliedposition: fixed. The next dropdown opened (any row, not just the one acted on) briefly renders in normal flex flow before floating-ui's async recompute lands, shoving its own trigger button sideways — so the panel ends up anchored to a phantom position instead of the real button.this.showwas tracked as a plain instance property, which the morph can leave desynced from the DOM (menu closed in the DOM, but the controller still thinks it's open), silently swallowing the first click on a dropdown right after it caused the morph.DS::PopovermirrorsDS::Menu's implementation and has the identical issue, so it's fixed the same way.Fix
menu.html.erb/popover.html.erb: addfixedto the content panel's static class list, soposition: fixedsurvives a morph resetting the inline style (onlyleft/topremain JS-driven).menu_controller.js/popover_controller.js: deriveshowfromcontentTarget.classList.contains("hidden")instead of tracking it separately, so it can never desync from the DOM.Before / After
Real app, fixture/demo data (not production data): disable "Checking Account" from its own dropdown, then open the unrelated "Credit Card" row's dropdown, which was never touched.
Test plan
turbo-rails/stimulus-railsgems andvendor/javascript) in an isolated harness, confirmed the exact failure mode, then confirmed it's resolved with the patched controller (panel lands pixel-exact on its trigger; previously-swallowed first click now opens on click 1).erb_lintclean on both templates.node --checkclean on both controllers.bin/rails test— 6022 runs, 0 failures, 0 errors.mainand the fix on this branch.Summary by CodeRabbit
Bug Fixes
Style