Guard toolbar overflow refresh against absent overflow control#1098
Guard toolbar overflow refresh against absent overflow control#1098romansklenar wants to merge 1 commit into
Conversation
#refreshOverflow runs inside a requestAnimationFrame scheduled from connectedCallback, the ResizeObserver, and the connected-attribute reconnect path. When the overflow control (.lexxy-editor__toolbar-overflow) is not in the DOM at the moment the frame fires, #overflowMenuButton is null, so #overflowMenuDropdown is null too and every method reading .children / .toggleAttribute throws a TypeError inside the rAF callback. This happens in the wild when a Turbo/idiomorph morph transiently detaches the toolbar template while the element stays connected, and when a page server-rendered by an older Lexxy (toolbar markup without the overflow control) is hydrated by newer JS. Bail out early when the overflow control is absent — there is nothing to compute without it, and setEditor() / the next ResizeObserver tick re-run the refresh once the template is in place, so behavior is preserved.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Prevents a production crash when the toolbar’s overflow refresh runs while the overflow control is temporarily missing from the DOM, and adds a regression test to ensure the scenario doesn’t log errors.
Changes:
- Add an early-return guard in
#refreshOverflow()when the overflow control is missing. - Add a Playwright regression test that removes the overflow control and asserts no console errors occur.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/browser/tests/formatting/toolbar.test.js | Adds regression coverage for overflow refresh when overflow control is absent and monitors console for errors. |
| src/elements/toolbar.js | Adds a defensive guard in #refreshOverflow() to bail out when overflow control isn’t available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await new Promise((resolve) => requestAnimationFrame(resolve)) | ||
| }) | ||
|
|
||
| expect(page).toHaveNoErrors() |
There was a problem hiding this comment.
toHaveNoErrors() isn't async here — it's a synchronous custom matcher defined in test/browser/helpers/assertions.js via expect.extend, and it returns a plain { pass, message } object (no Promise, no await inside). It evaluates fully at call time, so there's nothing to await. This matches the established usage elsewhere in the suite, e.g. test/browser/tests/attachments/gallery.test.js:223 and :400, which also call it un-awaited. Awaiting a non-thenable would be a harmless no-op but would wrongly imply the matcher is async, so I've kept it consistent with the rest of the suite.
| // The overflow control may be absent when a refresh fires: the toolbar | ||
| // template can be transiently detached during a Turbo/idiomorph morph, or a | ||
| // page server-rendered by an older Lexxy may be hydrated by newer JS. Bail | ||
| // out — setEditor() and the ResizeObserver re-run this once it's in the DOM. | ||
| if (!this.#overflowMenuButton) return |
There was a problem hiding this comment.
Good thing to check, but #overflowMenuButton isn't a cached reference — it's a getter that runs this.querySelector(".lexxy-editor__toolbar-overflow") live on every access. So in the case the comment describes (the control removed from the toolbar during a Turbo/idiomorph morph) the getter returns null and the guard bails correctly; there's no stale reference to go non-null.
The only remaining scenario is the toolbar element itself being detached while its overflow child stays intact. There querySelector still returns the element, but the downstream reads (.children, .toggleAttribute, offsetWidth, scrollWidth, getComputedStyle) are all safe on a detached node — they yield 0/empty rather than throwing, so no crash — and disconnectedCallback → dispose() already calls cancelAnimationFrame on the pending refresh when the element disconnects. So the null check is sufficient to prevent the production TypeError. Adding ?.isConnected would guard a non-crashing path and widen the change, so I've kept it to the minimal fix.
|
RDY for review. |
Problem
#refreshOverflowruns inside arequestAnimationFramescheduled from three lifecycle points (connectedCallback, theResizeObserver, and theconnected-attribute reconnect path). When the overflow control (.lexxy-editor__toolbar-overflow) is not in the DOM at the moment the frame fires,#overflowMenuButtonisnull, so#overflowMenuDropdownisnulltoo, and every method reading.children/.toggleAttributethrows aTypeErrorfrom inside the rAF callback.In the wild this surfaces as:
TypeError: Cannot read properties of undefined (reading 'children')TypeError: null is not an object (evaluating 'this.#overflowMenu.children')tagged
auto.browser.browserapierrors.requestAnimationFrame. It is the single biggest client-side error in a production app embedding Lexxy.When the overflow control is absent at refresh time
<lexxy-toolbar>element connected while its inner template is transiently detached — the overflow child is missing when the queued rAF fires.#overflowMenuButtonpermanentlynull→ crash on the first refresh.The optional chaining in
get #overflowMenuDropdown()only defers the throw to the first.children/.toggleAttributeread; it does not prevent it. The existingdisconnectedCallback → dispose() → cancelAnimationFramenarrows the disconnect race but does not cover these cases.Fix
Bail out of
#refreshOverflowearly when the overflow control is absent — there is nothing to compute without it, andsetEditor()/ the nextResizeObservertick re-run the refresh once the template is in place, so behavior is preserved.Test
Adds a regression test to
test/browser/tests/formatting/toolbar.test.jsusing the existing console-error hook (toHaveNoErrors()): it removes the overflow control, triggersrequestOverflowRefresh(), advances two frames, and asserts no page errors plus that the rest of the toolbar still renders. The test fails onmainwithCannot read properties of undefined (reading 'toggleAttribute')and passes with the guard.Full chromium browser suite: 484 passed.