Register a dialog when it is shown, not when it is constructed by wormeyman · Pull Request #285 · FactoryGameFan/factorio-blueprint-editor

wormeyman · 2026-08-30T20:09:36Z

Closes #280.

Dialog's constructor pushed onto s_openDialogs before any subclass body ran,
and close() is the only thing that takes an entry back out. So a subclass
constructor that throws after super() left an entry for a dialog that was
never shown and that nothing holds a reference to.

Reachable, not theoretical: icon names come out of the blueprint,
F.CreateIcon throws for a name FD does not have, data.json exports no planet
prototype at all (issue #231), and UIContainer.createEditor is a bare call
with no try above it. A display panel drawing a nauvis icon is the measured
path.

Both halves, and they are not alternatives

General - registration moved onto the pixi added event:

this.once('added', () => Dialog.s_openDialogs.push(this))

A constructor that throws never returns the object to be added, so it never
registers. All four construction sites are in UIContainer and call addChild
on the next line, so nothing can read the registry in the gap. once rather
than on, so a re-add cannot register the same object twice.

Narrow - DisplayPanelEditor's three F.CreateIcon calls now go through
F.SafeIcon, which warns naming the icon and returns an empty container. That
helper moved out of ToolsPanel.ts into functions.ts rather than being
copied.

Why not a try/catch in `UIContainer.createEditor`

That is the alternative the issue offered, and it was measured rather than
argued: a bare catch there does not clear the registry at all, because
s_openDialogs is protected - clearing it from outside would need a new
public escape hatch on Dialog. It also swallows the throw. createEditor now
carries a comment saying it stays a bare call on purpose: an editor that cannot
be built is a bug in that editor and should say so loudly.

Mutation record

mutation	row icon	condition	E keybind
`F.SafeIcon` back to `F.CreateIcon`	FAIL - page errors `[...named nauvis]` vs `[]`	FAIL - same, `vulcanus`	pass
`once('added')` back to a constructor push	pass	pass	FAIL - dialog count after `E`, 0 vs 1
both reverted (pre-fix)	FAIL	FAIL	FAIL
constructor push + bare try/catch in `createEditor`	pass	pass	FAIL at the control - 0 vs 2 on the page-error filter

Neither half covers for the other, and the last row is the rejected alternative
failing on its control rather than on the keybind.

A correction to the issue

The issue says E dies "for the rest of the session". That is wrong, and
I have corrected it there. closeLast() calls close() on the phantom, and
close() filters the registry on its first line, so the entry clears itself.
The real cost is one swallowed E or Escape per failed open. That changes
what a test has to do - a spec pressing E once would pass against the leak, so
the spec here presses twice.

Found while in here, not fixed

Recipe.ts has the same unguarded F.CreateIcon. An assembling machine
carrying a recipe name FD does not have still throws and opens no editor. Same
class of bug, deserves its own issue - it was the fixture for the general half
here.
Panel's resize listener leaks the same way. Its constructor calls
window.addEventListener('resize', ...) and only destroy() removes it, so a
throwing subclass leaves it attached. Not addressed, and no test here can see
it.
A space-location icon also breaks the display panel's own entity sprite with
Missing signal type mapping!. That one is caught and logged, so the entity
draws a placeholder. Adjacent to F.CreateIcon throws on planet icon names, taking down whatever called it #231.

Test plan

vp check clean - 0 warnings, lint or type errors across 188 files
vp test - 221 passed
tests/dialog-registry-leak.spec.ts (new) - 3 passed
With display-panel-editor, quick-actions, tools-panel, settings-pane-book-index - 26 passed
Full Playwright suite - 222 passed, matching --list
All four mutations above measured

🤖 Generated with Claude Code

https://claude.ai/code/session_01L7dF9eT5yMLqdJZ2rwytCz

`Dialog`'s constructor pushed `this` onto the static `s_openDialogs`, and `close()` was the only thing that took an entry back out. `super()` runs before any subclass body, so a subclass constructor that threw afterwards left an entry behind for a dialog that was never shown. `Dialog.anyOpen()` then answered true with nothing on screen, and the `E` keybind - which reads exactly that to choose between closing the top dialog and opening the inventory - went to the wrong branch. Two fixes, and they are not alternatives. Narrow: `DisplayPanelEditor` draws three icons whose names come out of the blueprint's `control_behavior.parameters`, and `F.CreateIcon` ends in a bare `throw` for a name FD does not have. No planet prototype is exported at all, so `nauvis`, `vulcanus`, `fulgora` and `gleba` all reach it, and 19 icon references in the committed corpus use one (issue #231). Those three calls now go through `F.SafeIcon`, so a missing icon costs the icon and a warning naming it, not the whole dialog. `F.SafeIcon` is `ToolsPanel`'s own `safeIcon`, moved into `controls/functions.ts` beside the two throwing icon builders it guards. Moved rather than copied: two copies of a fallback are two things to keep in step, and the one nothing exercises is the one that rots. General: registration moved from the constructor to the `added` event, which is the point a subclass cannot skip and cannot reach early. A constructor that throws never returns the object to be added, so it never registers. All four construction sites are in `UIContainer` and add on the next line, so nothing can read the registry in between. The rejected alternative - a try/catch in `UIContainer.createEditor` - is measured in the spec header: it does not clear the registry, since `s_openDialogs` is protected, and it buys the silence of every future broken editor. One correction to the issue text, measured. The phantom is not permanent. `closeLast()` calls `close()` on it and `close()` filters the registry before it destroys anything, so the entry clears itself. The cost is one swallowed `E` or `Escape` per failed open, not a dead keybind for the session. `tests/dialog-registry-leak.spec.ts` covers both halves and carries the mutation record. The general half needs its own fixture, because once the narrow half is in, no display panel can reach the throw any more: it uses an assembling machine carrying a recipe name FD does not have, which reaches an unguarded `F.CreateIcon` through `Editor`'s `Recipe` slot. That editor is still broken and this does not fix it - it is the fixture precisely because it is broken. `openDialogCount()` reads the pixi child count, so it cannot see a phantom entry at all. The only thing that can is the keybind that branches on it, which is also the thing the user loses. Closes #280

`F.CreateIcon` ends in a bare `throw` for a name FD does not have, and the names come out of the blueprint. `UIContainer.createEditor` has no `try` above it on purpose, so an unguarded call cost the user the whole editor: clicking the entity did nothing at all. Six sites, not the one #286 names. Four are icon slots and take `F.SafeIcon`, which #285 added for exactly this: - `Recipe.ts:38`, the site #286 names - `DisplayPanelIcon.ts:39` - the branch #285 did not reach. `DisplayPanel Editor` returns early when the panel is connected, and both display panel specs set `connect_to_logistic_network: true`, so an ordinary unconnected panel with a planet icon still lost its whole dialog - `Modules.ts:40` and `:68` - `Filters.ts:242` and `:251`, the amounts-on and amounts-off arms The other two are not icon slots and are why guarding `Recipe.ts` alone leaves the machine editor still refusing to open: - `Preview.generatePreview` called the *static* `OverlayContainer.createEntityInfo` bare, where `OverlayContainer`'s own instance method wraps the identical call. `Preview` is built by `Editor`'s base constructor, so that throw cost *every* editor its dialog. - `EntityInfoPanel.updateVisualization` calls `getModule`, which throws by design (#55). It runs from `pointerOverEventHandler`, so an unknown module threw on hover, before any click. Those two warn with `console.warn` rather than `G.logger`, which is the toast: both repeat on every hover or redraw, and the icon slots already raise one toast when the dialog opens. `tests/editor-icon-guards.spec.ts` covers all six, one case each, mutation checked - every guard reverted in turn fails at least one test, and the two `Filters` arms need both the chest and splitter cases. `tests/dialog-registry-leak.spec.ts` borrowed the machine editor's throw as the fixture for its registry half, and said so. That fixture is gone, and replacing it with another live bug would mean a test that argues against fixing it. It now uses a `ThrowingDialog` behind a `__fbe_test` hook - constructed, never added, so registration on the `added` event leaves nothing behind. Re-measured: reverting the registry fix still fails that test at the keybind, 0 against 1. Closes #286. Closes #288. Claude-Session: https://claude.ai/code/session_016ZkWutgXP9XeVoh2RaAX3S Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

…uilt `Panel`'s constructor put a `resize` listener on `window` and `destroy()` was the only thing that took it off. A subclass constructor that throws after `super()` returns never hands back the object, so nothing can ever call `destroy()` on it: the listener stayed on `window` for the life of the page, held the half-built panel and its whole pixi subtree alive, and called `setPosition()` on that object at every resize. The same shape as #280, one constructor lower down - `Editor extends Dialog extends Panel`, so `Panel`'s is the first constructor to run and the last thing a throwing subclass can undo. #280's own measured path (`DisplayPanelEditor` drawing a planet icon) and #286's (`Recipe.ts`) both throw well after that line. Registration moves onto the pixi `added` event and comes off on `removed`, which is the move #285 made for the dialog registry. `added`/`removed` rather than `once`, so a panel that leaves the tree and returns stays balanced; re-adding is safe because the DOM ignores an identical (type, listener) pair. `destroy()` still removes it too, for a panel destroyed while already detached. `tests/panel-resize-listener.spec.ts` counts the listeners directly, by wrapping `addEventListener`/`removeEventListener` before the page's scripts run - nothing else in tests/ can see this, since a leaked listener changes no behaviour at all, it only retains an object. It tracks listener functions rather than counting calls, because the DOM dedupes an identical pair. The third test is the load-bearing one: a "fix" that simply never registers passes the other two perfectly. Measured - deleting the `added` line alone leaves tests 1 and 2 green and fails test 3, along with both of tools-panel.spec.ts's narrow-viewport cases. Closes #287. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZkWutgXP9XeVoh2RaAX3S

…uilt (#290) `Panel`'s constructor put a `resize` listener on `window` and `destroy()` was the only thing that took it off. A subclass constructor that throws after `super()` returns never hands back the object, so nothing can ever call `destroy()` on it: the listener stayed on `window` for the life of the page, held the half-built panel and its whole pixi subtree alive, and called `setPosition()` on that object at every resize. The same shape as #280, one constructor lower down - `Editor extends Dialog extends Panel`, so `Panel`'s is the first constructor to run and the last thing a throwing subclass can undo. #280's own measured path (`DisplayPanelEditor` drawing a planet icon) and #286's (`Recipe.ts`) both throw well after that line. Registration moves onto the pixi `added` event and comes off on `removed`, which is the move #285 made for the dialog registry. `added`/`removed` rather than `once`, so a panel that leaves the tree and returns stays balanced; re-adding is safe because the DOM ignores an identical (type, listener) pair. `destroy()` still removes it too, for a panel destroyed while already detached. `tests/panel-resize-listener.spec.ts` counts the listeners directly, by wrapping `addEventListener`/`removeEventListener` before the page's scripts run - nothing else in tests/ can see this, since a leaked listener changes no behaviour at all, it only retains an object. It tracks listener functions rather than counting calls, because the DOM dedupes an identical pair. The third test is the load-bearing one: a "fix" that simply never registers passes the other two perfectly. Measured - deleting the `added` line alone leaves tests 1 and 2 green and fails test 3, along with both of tools-panel.spec.ts's narrow-viewport cases. Closes #287. Claude-Session: https://claude.ai/code/session_016ZkWutgXP9XeVoh2RaAX3S Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

wormeyman force-pushed the fix/280 branch from 0c34a5f to f3aa45d Compare August 30, 2026 20:13

wormeyman merged commit 3785461 into wormeyman-space-age-support Aug 30, 2026
9 checks passed

Navigation Menu

Uh oh!

Register a dialog when it is shown, not when it is constructed - #285

Register a dialog when it is shown, not when it is constructed#285
wormeyman merged 1 commit into
wormeyman-space-age-supportfrom
fix/280

wormeyman commented Aug 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wormeyman commented Aug 30, 2026

Both halves, and they are not alternatives

Why not a try/catch in UIContainer.createEditor

Mutation record

A correction to the issue

Found while in here, not fixed

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Why not a try/catch in `UIContainer.createEditor`