Add a ToolsPanel with entity-info toggle/import string/export string/export image/undo/redo UI actions

[koenigstag]

Re-submission of #221, recreated after the repo detach.

Summary

• Expose import-replace, import-append, export-to-string and export-to-image — actions that only ever existed as keyboard shortcuts (paste, Ctrl+Shift+V, copy, Ctrl+S) — as buttons, plus Undo/Redo (Ctrl+Z/Ctrl+Y).
• Added ImportDialog (textarea + Replace/Append buttons with descriptions) and ExportDialog (pre-filled, pre-selected textarea) as a clipboard-free alternative, for browsers that block navigator.clipboard or a string that needs to be seen/edited.
• Renamed WiresPanel to ToolsPanel, since it now holds import/export/undo/redo alongside the wire slots.

Layout notes

• The panel wraps into a column-major grid instead of a single wide row — a wide single row runs part of it under .toasts-container (position: fixed; right: 0), confirmed by clicking a widened-panel slot and finding the click reached the toast instead of the button.
• Undo/Redo call G.bp.history.undo()/redo() directly (same as their keybinds in Editor.ts); the four clipboard/file actions go through a new QuickActions interface injected via Editor.init(), mirroring the existing Logger injection.
• Icons: game GUI sprites (import_slot/export_slot) and the signal-anticlockwise-circle-arrow/signal-clockwise-circle-arrow virtual signals for Undo/Redo — the only undo/redo-shaped icons anywhere in vanilla Factorio's data.

Test plan

• vp check / vp test pass
• Manually verified all quick actions, both dialogs, and Undo/Redo end-to-end against the dev server

[koenigstag]

PR recreated after the repo detach (was #221). Ready for review — waiting on any fixes/comments.

[wormeyman]

Reviewed at f90e865. Fourteen inline findings below, plus one convention note.

Two of them I'd treat as blocking, and both surface on first use of the feature - consistent with the test plan's manual-verification box still being unchecked:

1. ExportDialog's pre-selection never fires (ExportDialog.ts:43). select() runs in a microtask while the textarea is still display: none, so Ctrl+C falls through the document copy listener to navigator.clipboard.writeText - the exact API the dialog exists to bypass.
2. exportGuardResult doesn't report a guard, it runs both actions (packages/website/src/index.ts:339) - a real clipboard write and a real PNG download - and __fbe_test is assigned unconditionally, so it ships in production builds.

Four more are reachable UX defects: Escape can't close either dialog once the field has focus, Replace/Append destroy the user's hand-edited text before the async import can fail, the Alt highlight is an opaque fill inserted above the hover and active sprites, and the export field is editable and silently stale. The rest are docs that don't match their code, one dead function, and test-side duplication.

Convention (repo CLAUDE.md, Git and PR Conventions): the PR title carries #221, so this squashes to ...UI actions- #221 (#242) - the rule is to keep the issue number out of the subject and put Closes #N in the body. The body says "Re-submission of #221" but has no Closes #221, so merging won't close the issue. Separately, the title's "show details" doesn't name anything in the diff - that slot is labelled ALT and toggles entity info overlays.

Nice work on the comment density throughout - the reasoning left at ROWS, maxLength and WireSlot is the right shape for this repo. Several findings below are that the prose and the code drifted apart, not that the prose was unwelcome.

[wormeyman]

Reviewed 7cd1e36e against the fourteen findings from my last pass. Ten are fixed. I checked each one against the code rather than against the commit message. Four are untouched with no note either way, and one of the fixes introduced a new problem that I measured.

The two blocking ones are properly fixed

select() now defers to UPDATE_PRIORITY.UTILITY, which runs below Application's own LOW-priority render step, so _updateDOMInput has shown the textarea by the time it fires.

exportGuardResult is now !bp.isEmpty(). I checked that against both functions: it is the exact condition exportString and exportImage each open with, so it reports the guard without being able to perform either action.

Both spec files ran in CI rather than being collected and skipped (shards 3 and 4).

New: the freshness fix costs 310 ms every 500 ms

I asked for this one, so the cost is on me, but it is worth fixing before merge.

ExportDialog re-encodes on a 500 ms interval now, and encode() (bpString.ts:249) is fully synchronous inside its Promise executor. serialize(), JSON.stringify, pako.deflate and base64 all run on the main thread before resolve() is called.

Measured on the corpus's own largest blueprint, test-blueprints/EARN/pocket-base-space-age-v22.1.2.txt, a 2.42 MB string holding 29.95 MB of JSON:

stringify + deflate + base64: median 310 ms (min 300, max 329, 7 runs)

That is a lower bound. It leaves out Blueprint.serialize(), which I cannot run outside a browser. And m_LastEncodeAt is stamped at the top of refreshText, before the work rather than after it, so the interval is start-to-start: about 190 ms of idle between 310 ms stalls. Call it 62% of the main thread and 19 dropped frames per cycle.

The dialog deliberately does not block the canvas, which is why the field could go stale at all. So this makes the editor stutter during the one activity the freshness fix exists to support.

History already keeps a historyIndex that moves on every commit, undo and redo (History.ts:198). It is private today, but exposing it, or any monotonic counter, turns the tick into an integer comparison that re-encodes only when something actually changed. That costs nothing while the user is reading rather than editing.

Four findings unaddressed, with nothing said either way

#	Site	State
6	ToolsPanel.ts:151	The doc still promises "a blank square the same size as a real icon". The code still returns a childless new Container(), which draws nothing.
8	ToolsPanel.ts:314	Math.min still has no lower bound, so it goes negative below about 212px, which is the failure it was added to prevent, mirrored. The doc still calls the quickbar overlap "click through" when UIContainer adds ToolsPanel after the quickbar.
9	controls/functions.ts:406	File untouched. Still scale.set(maxSize / width), so height never reaches the scale, and data.width ?? size still passes a literal 0 through the === undefined guard to Infinity.
7	ToolsPanel.ts:262	Half done. placeCells is extracted and called now, but generateSlots still has no call site, and the comment's factual error survives word for word: Container.destroy() with no options does not destroy children either.

I am not asking for all four before merge. 6 and 7 are comment corrections. 8 and 9 are each a one-liner, and 9 sits in a helper exported on F for general use.

Smaller

noopQuickActions.readClipboardText both logs through G.logger and rejects, where the other four log and resolve to a falsy value. ImportDialog's Paste button catches the rejection and logs it again, so a missing quickActions raises two toasts for that one action and one for each of the others.

Convention: the - #221 is gone from the title, thank you. The body still has no Closes #221, and that is the half GitHub reads to close the issue. The title's "show details" also still names nothing in the diff. That slot is ALT.

[wormeyman]

Reviewed c8f505ba and 98f09316 together. Nine of the fourteen findings from my earlier passes are now fixed and I checked each against the code rather than the commit message. The performance fix is right, and all four of the older ToolsPanel/functions.ts findings are genuinely closed.

I verified everything locally, because CI has never run either commit - see the first section.

Two things need fixing before merge, and one of them I found by running the suite rather than reading it.

CI is red repo-wide, and it is not this PR's fault

All four Playwright shards and checks fail in 14-29s at Set up Vite+:

sha256sum: WARNING: 1 computed checksum did NOT match

vite-plus 0.3.0 shipped on 2026-08-24 and rewrote install.sh, so the pinned hash stopped matching under an unchanged VP_VERSION. That is the pin working. #260 fixes it and is already green on checks. Until it lands, a red Set up Vite+ here says nothing about this PR.

Verified fixed

c8f505ba - ExportDialog polls History.revision (an integer compare) and debounces a re-encode 500 ms after the blueprint stops changing. The idle 310 ms stall every 500 ms is gone. I also went looking for the staleness hole this approach could open and it is not reachable: loadBlueprint calls Dialog.closeAll() before anything can observe a new blueprint's reset historyIndex, and history.reset() runs only in the Blueprint constructor.

98f09316 - all four older findings, checked individually:

#	Fix	Verified
6	safeIcon doc now says "empty, childless Container"	Matches the code
7	generateSlots doc no longer claims destroy() cascades	Correct, and placeCells really does child.destroy() on each removed child, so the new claim holds too
8	Math.max(0, Math.min(...))	Mutation-checked - removing it gives x = -62 at 150px
9	!width || !height, and maxSize / Math.max(width, height)	Catches the literal 0, and fits both axes

Local gates on 98f09316: vp check clean (0 lint/type errors), vp test 180/180, full Playwright 212 passed, 1 failed - and that one failure is the subject of the next section.

Blocking: the new ToolsPanel test is flaky, and so is the one it was modelled on

tools-panel.spec.ts:155 failed in a clean full-suite run on unmutated code, with Received: 861.

Measured in isolation:

Test	Result
:155 "does not run off the left edge" (new in 98f09316)	2 failures in 5 runs
:140 "stays on screen at a narrow viewport width" (pre-existing)	3 failures in 6 runs

So this is a pre-existing flake that the new test copied, not one it introduced - but neither has ever run in CI, so both would land and then fail on shard 4 roughly 40% of the time, in a spec nobody touched.

The mechanism, and the fix I measured, are inline.

Blocking: the ExportDialog test does not guard the debounce, and says it does

Deleting the debounce entirely leaves the test passing, in 2.1s. Inline.

Two doc comments overstate their code

History.revision and ExportDialog.encodeCount. Comment-only, both measured, both inline. revision is new public surface on a core class, which is why it is worth getting exactly right.

Still open from earlier passes

noopQuickActions.readClipboardText rejects where its four siblings resolve to a falsy value, so a missing quickActions raises two toasts for that one action and one for each of the others. And the body still has no Closes #221, which is the half GitHub reads to close the issue.

[wormeyman]

@claude review

[claude]

Claude finished @wormeyman's task in 3m 7s —— View job

---

Review complete

• Gather context (PR body, prior review threads, CLAUDE.md conventions)
• Check what has changed since the last reviewed commit (98f09316)
• Verify status of previously-flagged unresolved items
• Review the new commits for fresh issues
• Post final review

Reviewed c99645b and ea5984d, the two commits landed since the last review round (98f09316). CI is now green end-to-end on the current HEAD (4e9a9a1) — checks, both Rust exporter jobs, and all 4 Playwright shards passed (run 32891939061), so #260's vite-plus checksum fix did land and this branch is being tested for real now.

Verified fixed (checked against the code, not the commit message)

#	Finding	Fix
History.revision doc	Claimed "exactly ±1 on every commit"	Now correctly documents the nested-transaction, empty-transaction, and trim-boundary exceptions, and explicitly scopes the guarantee to ExportDialog's !== use
ExportDialog.encodeCount doc	Claimed it counts completed serialize+deflate	Now correctly says it counts refreshText calls, which bump synchronously before the async encode resolves
Debounce test only proved "eventually re-encodes"	Passed even with the debounce deleted	New test asserts a delete + undo inside one 500ms window settles at exactly 2 encodes, not 3 — verified this actually fails under the debounce-removed mutation
noopQuickActions.readClipboardText rejected, siblings resolve	Double-toast on a missing quickActions	Now resolves to '' like its siblings, with the reasoning for why an empty string is a safe stand-in written down
Flaky tools-panel.spec.ts:155 (150px, off-left-edge)	2/5 failures, stale pre-resize read	Now polls toolsPanelBounds().x for toBe(0) — this does force a real wait, since the stale (pre-resize) x is 861, not 0
Flaky debounce-burst test in CI shards	page.waitForTimeout(200) raced the same 500ms debounce it was observing, only reproduced in a loaded 54-test shard	Intermediate "still 1" check removed; the final "settles at 2" checks already catch the same mutation without depending on wall-clock precision under load
Control+z in quick-actions.spec.ts	Flagged by the (newly-merged) spec-modifier-keys.test.ts gate	Changed to Control+KeyZ with an ALLOWLIST entry matching the existing precedent

One remaining item, only partially fixed

tools-panel.spec.ts's other viewport test (ToolsPanel stays on screen at a narrow viewport width, 800px) still has the race it claims to have fixed.

await page.setViewportSize({ width: 800, height: 720 })

await expect
    .poll(() => page.evaluate(() => window.__fbe_test.toolsPanelBounds().x))
    .toBeGreaterThanOrEqual(0)

const bounds = await page.evaluate(() => window.__fbe_test.toolsPanelBounds())
expect(bounds.x + bounds.width).toBeLessThanOrEqual(800)

setPosition() is x = max(0, min(screen.width/2 + 221, screen.width - this.width)). At the stale pre-resize width (1280, the default viewport), x = max(0, min(861, 1068)) = 861 — which already satisfies .toBeGreaterThanOrEqual(0). So this poll's condition is true whether or not the resize has actually landed, meaning it resolves on its very first check and adds no real wait. The un-polled read right after it (bounds.x + bounds.width <= 800) is exposed to exactly the same race the original single-read version had — the one originally measured at 3 failures in 6 runs.

The sibling 150px test's fix works precisely because its target (x === 0) is not true in the stale state (861 ≠ 0), so the poll is forced to actually retry until the resize lands. This test needs the same shape — poll the joint condition, not just half of it:

await expect
    .poll(() =>
        page.evaluate(() => {
            const b = window.__fbe_test.toolsPanelBounds()
            return b.x >= 0 && b.x + b.width <= 800
        })
    )
    .toBe(true)

This was actually the exact fix suggested in the prior review round for this test; the fix that landed applied it to the other (150px) test but not this one. It's low severity — a test-only flake with roughly a coin-flip failure rate under load, same class as the one already fixed — but worth closing since it's a one-line change and the mechanism is now well understood.

Fix this →

Still open from earlier rounds (non-blocking, convention only)

• The PR body still has no Closes #221 — that's the line GitHub reads to close the original issue on merge; "Re-submission of Add a ToolsPanel with show details/import string/export string/export image/undo/redo UI actions #221" alone won't do it.
• The PR title's "show details" still doesn't name anything in the diff — that slot is labelled ALT and toggles entity info overlays.

Overall

Nine of the ten items from the last review pass are cleanly fixed and verified against the code. The tenth (the 800px viewport test) got half the intended fix — the mechanism is right, just applied to the wrong assertion shape on that particular test. Given everything else here has already been through three rounds of measured, verified fixes, this is close to mergeable pending that one-liner and the two convention notes above.
· Branch

[wormeyman]

Re-reviewed at 9338da48. Almost everything from the previous three rounds is fixed with the mechanism removed rather than the wording satisfied, which is the harder and better way to do it. The newest commit is a genuine deterministic fix, not a settle-wait: x >= 0 alone was already true at the stale 1280px layout, so the old poll resolved on its first check and added no wait at all. The joint condition can actually fail. That is the right shape.

Three things left. One is user-facing.

Closing either dialog leaves Ctrl+C and Ctrl+V silently dead

Both handlers open with if (document.activeElement !== CANVAS) return (packages/website/src/index.ts:305 and :311). When a dialog is destroyed, TextInput._onRemoved removes the focused <input> from document.body, which resets document.activeElement to <body>. Nothing in packages/ ever focuses the canvas back. The only .focus() calls in the whole tree are TextInput.ts:195 and :203.

So: open Export, which focuses the textarea by design, press Escape, then press Ctrl+C. Nothing happens. No toast, no error. The user gets it back only by clicking the canvas.

The canvas already has tabindex="1", so a CANVAS.focus() on dialog close is the whole fix. quick-actions.spec.ts:242 presses Escape and then asserts only openDialogCount() === 0, so no test can see this today.

The empty-field test cannot fail for its stated reason

quick-actions.spec.ts:185 is written to cover runImport's up-front empty guard. Delete that guard and every assertion still passes. importReplace('') falls through to new URL('https://') and throws, importReplace's own .catch swallows it, so there is no pageerror; the dialog stays open; the blueprint is untouched. The only observable difference is the toast text. Asserting that would close it.

One comment is wrong in a way that invites a real bug

ToolsPanel.ts:319 says the quickbar overlap below ~866px is "the same trade-off a real user can still click through". UIContainer.ts:40 adds the panel after the quickbar, so it draws on top, and pixi hit-tests children in reverse. Those slots are dead, not click-through. Someone reading that comment and widening the panel or relaxing the clamp would make it worse, and nothing in tests/ hit-tests over the quickbar, so it would land green.

Take or leave: the panel is a noticeably larger permanent dead zone over the canvas than WiresPanel was, and it is added after dialogsContainer, so at short viewport heights it covers the bottom of a centred dialog.

[wormeyman]

Details in the comment above. Short version: almost everything from the previous rounds is fixed with the mechanism removed rather than the wording satisfied, and the newest commit is a real deterministic fix rather than a settle-wait.

Three things left. The one worth doing before merge is the focus bug: closing either dialog leaves Ctrl+C and Ctrl+V silently dead, because nothing focuses the canvas back. CANVAS.focus() on close is the whole fix.

[wormeyman]

I drove this one rather than only reading it: a worktree at the PR head, both dev servers up, and mutations applied one at a time.

The mechanism is right. The canvas carries tabindex="1" (packages/website/index.html:90) and G.app.canvas is that same element, so it is the one the clipboard guards compare against. close() filters s_openDialogs on its first line, so anyOpen() after destroy() is not off by one. destroy() removes the <input> synchronously, so nothing can undo the focus() afterwards. #editor has outline: none, so no focus ring appears.

The two tests carry their weight. I measured the mutation table rather than reasoning about it:

mutation	test 1 (last dialog)	test 2 (one still open)
remove G.app.canvas.focus()	fails	passes
drop the !anyOpen() guard	passes	fails

Each kills exactly one. The end-to-end claim holds too: after opening and closing ExportDialog with Escape, a real Ctrl+C raises the "copied to clipboard" toast with no canvas click first.

One regression

With a dialog open, the BP Book Index arrow keys stop working after one press.

settingsPane.ts:47 hangs its own keydown handler on that input. ArrowUp calls changeBookIndex -> editor.loadBlueprint -> Dialog.closeAll() (Editor.ts:415) -> the last close -> G.app.canvas.focus(). The live <input> loses focus, so the second arrow reaches the editor's keybinds instead of the box.

	focus after 1st arrow	index after 1st / 2nd
this PR	CANVAS	1 / 1
commit reverted	INPUT	1 / 2
no dialog open (control)	INPUT	1 / 2

Nothing in the suite can see it. git grep ArrowUp tests/ is empty.

Fix

Only claim focus that is genuinely orphaned:

-        if (!Dialog.anyOpen()) G.app.canvas.focus()
+        if (!Dialog.anyOpen() && document.activeElement === document.body) {
+            G.app.canvas.focus()
+        }

Reading document.body separates "the field that had the focus was just destroyed" from "something else still holds it". The first is the case the hand-back is for, and the only one it should touch. The comment above the guard needs the second half explained; I can hand over my wording if that helps.

With this applied, vp check, vp test (188) and the full Playwright suite (218) all pass. Reverting just the activeElement half fails the second test in the spec below and nothing else.

tests/settings-pane-book-index.spec.ts (new file)

import { test, expect } from '@playwright/test'
import {
    encodeBlueprintBook as encodeBook,
    packVersion as version,
} from './helpers/encode-blueprint'
import { loadBlueprint, waitForEditor } from './helpers/fbe-test-api'

/*
    The settings pane's BP Book Index box, and the first coverage its arrow
    keys have ever had - `git grep ArrowUp tests/` was empty before this file.

    `packages/website/src/settingsPane.ts` hangs its own `keydown` listener on
    that dat.gui `<input>` so ArrowUp/ArrowDown step the index. Stepping is
    the whole reason the listener exists, and stepping means the *second*
    press has to work as well as the first - which is what makes this
    reachable at all, and what nothing else here can see.

    Written for a regression the #242 focus hand-back introduced (see
    `Dialog.close()`). Each step calls `changeBookIndex` ->
    `editor.loadBlueprint` -> `Dialog.closeAll()`, so with any dialog open the
    first arrow closes it, and an unconditional `G.app.canvas.focus()` on that
    close pulled the focus off the live <input>. The second arrow then reached
    the editor's keybinds instead of the box. Measured 0 -> 1 -> 1 against the
    0 -> 1 -> 2 the control gives.

    Two things this spec is shaped by.

    The pair is a real pair, not a test and a restatement. Only the second
    test fails against the unconditional guard; the first passes there, and
    both pass with the focus hand-back deleted outright. So the control says
    the arrows work at all and the treatment says a dialog does not break
    them, and neither one alone distinguishes the three states.

    It does not call `suppressOverlays`. That helper sets `pointer-events:
    none` on `.dg.main`, which is this pane - the very thing the test has to
    click. The pane is bottom *left* and the toasts are bottom *right*, so
    there is nothing here for the suppression to protect against anyway.

    The book entries carry a different entity count each, and that is the
    synchronisation. `setValue` writes the box synchronously and only then
    starts an async load, so waiting on the box's own value proves nothing
    about whether the load - and the `closeAll` inside it - has run yet.
    Pressing the second arrow into that gap passes whatever the guard does.
    `entityContainerCount()` moves only when a load finishes, and it names
    *which* entry finished.
*/

const VERSION = version(2, 0, 55)

const chests = (n: number): Record<string, unknown>[] =>
    Array.from({ length: n }, (_, i) => ({
        entity_number: i + 1,
        name: 'wooden-chest',
        position: { x: i + 0.5, y: 0.5 },
    }))

/** Entry i holds i+1 chests, so entityContainerCount() names the loaded entry. */
const BOOK = encodeBook({
    item: 'blueprint_book',
    version: VERSION,
    active_index: 0,
    blueprints: [0, 1, 2].map(index => ({
        index,
        blueprint: { item: 'blueprint', version: VERSION, entities: chests(index + 1) },
    })),
})

type Page = import('@playwright/test').Page

const bpIndexInput = (page: Page) =>
    page.locator('.dg .property-name:has-text("BP Book Index")').locator('..').locator('input')

const activeTag = (page: Page): Promise<string | undefined> =>
    page.evaluate(() => document.activeElement?.tagName)

/** Waits for the load the arrow started to finish, by the entry it landed on. */
const waitForEntry = (page: Page, index: number): Promise<unknown> =>
    page.waitForFunction(n => window.__fbe_test.entityContainerCount() === n, index + 1)

test.beforeEach(async ({ page }) => {
    await waitForEditor(page)
})

test('CONTROL: the arrow keys step the book index twice over, with no dialog involved', async ({
    page,
}) => {
    await loadBlueprint(page, BOOK)
    const input = bpIndexInput(page)
    await input.click()
    expect(await activeTag(page)).toBe('INPUT')

    await page.keyboard.press('ArrowUp')
    await waitForEntry(page, 1)
    expect(await activeTag(page)).toBe('INPUT')

    await page.keyboard.press('ArrowUp')
    await waitForEntry(page, 2)

    await expect(input).toHaveValue('2')
})

test('a dialog closing on the first arrow does not steal the focus off the index box', async ({
    page,
}) => {
    await loadBlueprint(page, BOOK)
    await page.evaluate(() => window.__fbe_test.openExportDialog())
    expect(await page.evaluate(() => window.__fbe_test.openDialogCount())).toBe(1)

    const input = bpIndexInput(page)
    await input.click()
    expect(await activeTag(page)).toBe('INPUT')

    await page.keyboard.press('ArrowUp')
    await waitForEntry(page, 1)

    // Load-bearing: without this the test could pass on a dialog that never
    // closed, which is the one state that cannot exercise the guard at all.
    expect(await page.evaluate(() => window.__fbe_test.openDialogCount())).toBe(0)
    expect(await activeTag(page)).toBe('INPUT')

    await page.keyboard.press('ArrowUp')
    await waitForEntry(page, 2)

    await expect(input).toHaveValue('2')
})

The guard's stated reason is not true

The comment says that while another dialog is open, "its field is what the keyboard should be talking to". It is not. Only ExportDialog focuses its own field (ExportDialog.ts:168); ImportDialog never does.

I drove the exact case the second test sets up, open Import, open Export, Escape, and document.activeElement.tagName is BODY, with the Import textarea on screen and unfocused. Typing reached no field at all. The keys went to the editor's keybinds, and the E in my test string opened the inventory dialog.

That state has the same dead keyboard the commit removes elsewhere. It predates the commit, so it is not a regression. But the second test pins it as intended behaviour, and the comment justifies it with something the code does not do. Either correct the comment or focus the surviving field.

Minor

• The commit message says TextInput's two calls are "the only .focus() in the tree". Two more live in tests/. The in-code comment says "in the package" and is correct.
• The mutation table is in the commit message. paste-placement.spec.ts and splitter-wires.spec.ts both keep theirs in the file header, which is what someone reads before deleting a test that looks like it always passes.
• page.evaluate(() => window.__fbe_test.openExportDialog()) now appears 7 times raw, while openImportDialog has a helper.

One more that I could not verify: s_openDialogs leaks if a dialog subclass constructor throws, which would leave anyOpen() permanently true and disable the hand-back for the rest of the session. I found no reachable path to it.