Take the command palette off the first screen, and fix a grid test race - #96
Merged
Conversation
The engine split, attempted properly rather than argued about. Two things
came out of it: one measured win, and a much sharper answer about the rest.
The win. The command palette was the last overlay still arriving with the
first screen. Its catalogue enumerates every control the forge has, so
importing it imported the letter recipes and the part specifications. It is
deferred now, like the seven overlays already were, and warmed with them at
idle. The shortcut that opens it is a hook and stays. The entry chunk goes
from 965 kB to 940, and from 303.7 kB gzipped to 295.7.
That is eight kilobytes gzipped, and the reason it is not eighty is worth
writing down, because I had the shape of this wrong before.
I said the forge was reached through five separate edges and that all five
would have to go. That was true but it was not the useful description. Four
of them are shadowed by the fifth: App.tsx calls `useForge()` on every
render, and `forge-store.ts` imports twenty-eight document-editing functions
at module scope. Breaking the others changes nothing while that stands, and
this commit proves it -- `castOf` and its three siblings moved into a leaf
module and `toTypeface` became a dynamic import, and the build did not move
by a single byte, because forge-store was pulling both anyway.
And `useForge()` is not an accident. App watches `forge.revision` so the
session gets written down when a drawing changes. Leaving it out is the bug
the comment above `revisions` describes: a store whose work is never saved,
silently. So the store cannot simply be deferred.
What would actually do it, for whoever picks this up:
- Move the twelve forge callbacks off `Shell` and into the palette, which
is deferred as of this commit. App holds them today and nothing but the
palette calls them.
- Give the revision counter a module of its own for App to subscribe to,
so watching a drawing change does not mean importing the thing that
draws.
- `forgeStore.refresh()` and `.snapshot()` are the two remaining uses in
App, both in handlers, both reachable with a dynamic import.
That is about two hundred lines across four files and it changes the
palette's contract, which is why it is not in this commit. The prize is
seventy-eight kilobytes gzipped, measured, not guessed.
The two changes that bought nothing today are kept because they are correct
and they are what that work would start from: reading a cut off a document
should not require the engine, and neither should having a `toTypeface`
available for a button nobody has pressed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULPrwXh4M5YPRkd54xJKLM
Two tests in handles.spec.ts turn the grid on and then read the letter as
proof of what a grid-built letter looks like:
await panel.getByRole("switch", { name: "Build on a grid" }).click();
await expect(page.locator("[data-forge-cells]")).toBeVisible();
const laid = await outline();
[data-forge-cells] is the grid's editor and it appears before the alphabet
has been redrawn out of cells, so `laid` was sometimes still the skeleton.
Clearing the grid puts the letter back to its skeleton -- so the poll on the
next line, waiting for the outline to stop being `laid`, was waiting for a
change that had already been and gone. It could only time out.
The comment above that poll already said "It went flaky on CI before it went
red", which was the right observation attached to the wrong line: the poll
was never the problem, the reading before it was.
They now take the letter before the switch is thrown and wait for it to stop
being that, which is what `laid` was always meant to mean.
Found by the full suite on the commit before this one, at one failure in two
hundred and forty-three. It passed alone, and it passed with its own file run
in parallel; only the whole suite was slow enough to lose. Deferring the
command palette shifted the timing of the first screen enough to expose it,
which is the honest reason it turned up now rather than a reason to believe
it was new.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULPrwXh4M5YPRkd54xJKLM
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ibrahimweng
added a commit
that referenced
this pull request
Sep 5, 2026
`src/forge/` is 287 kB of letter recipes, part specifications and geometry, and it arrived with the first screen of everybody who opened the application -- including the people who only ever edit a font somebody else made. Deferring the palette in #96 moved nothing, because a dozen other things were importing it too. Static edges from App.tsx into the engine: 20 modules to 5. The five that remain are `shapedInk`'s own reach, which is on the synchronous path outlines are resolved on; moving that means making the outline path async and is a different job. First load -- the entry chunk plus everything it statically imports: before 1194.68 kB 379 kB gzipped after 781.54 kB 257 kB gzipped state/drawn.ts is the new piece: the drawing seen from outside the thing that draws. Six components wanted one small fact from the forge store -- a family name in the toolbar, whether undo was available, a letter handed back from the editor, a snapshot for the session -- and each imported the whole engine to ask. The store now says what it is from the one place its state is written, and the clicks fetch it when pressed. The count is owned there rather than mirrored, so it cannot drift from the store's revision: a second copy kept in step by hand is exactly the silent no-save failure the note above `revisions` describes. project/format.ts pulled twelve of the twenty on its own, through two functions. `hasDrawing` became `worthKeeping` in forge/document.ts, where the bases it compares against already live, and the forge store is what asks it. `whole` moved to `forgeStore.restore`: reading a project no longer needs the engine, and putting one back is already loading it. The four side panels are deferred and warmed alongside the views they stand beside. forge/script.ts's `scatterOf` -- a hash function both engines share, and its own comment says it is not a piece of the forge -- moved to font/scatter.ts, so the Trace store stops pulling eighteen hundred lines of brush script to hash a one-character name. And `baseFor` moved to library/measure.ts, since which base a font lands on is a reading of the measurement rather than a thing that needs the twenty bases in hand. Tests followed the code: worthKeeping's five moved to forge/keeping.test.ts, older.test.ts goes through the read-then-fill split the application now uses, and drawn.ts has nine of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULPrwXh4M5YPRkd54xJKLM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits. One is an attempt at deferring the drawing engine, which got part of the way and produced a much better answer about the rest. The other is a test race that the first one exposed.
The command palette waits now
It was the last overlay still arriving with the first screen. Its catalogue enumerates every control the forge has, so importing it imported the letter recipes and the part specifications along with it.
It is deferred now, in the same way the seven other overlays already were, and warmed at idle with them. The keyboard shortcut that opens it is a hook, so that stays where it was.
The entry chunk goes from 965 kB to 940, and from 303.7 kB gzipped to 295.7.
Why that is eight kilobytes and not eighty
I have said before that the forge is reached from the always loaded half through five separate edges, and that all five would have to go. That was true, and it was not the useful way to describe it. Four of those edges are hidden behind the fifth.
App.tsxcallsuseForge()on every render, andforge-store.tsimports twenty-eight document editing functions at the top of the file. While that is true, breaking any of the other edges changes nothing.This branch demonstrates that rather than asserting it. It moves
castOfand its three siblings into a leaf module and turnstoTypefaceinto a dynamic import, and the build does not move by a single byte, because the forge store was pulling both of them in anyway.useForge()is also not an accident. App watchesforge.revisionso that a drawing gets written into the session when it changes. Leaving a store out of that list is the bug the comment aboverevisionsdescribes, where work is silently never saved. So the store cannot simply be deferred.What would finish it
For whoever picks this up, including me:
Shelland into the palette, which is deferred as of this branch. App holds them today and nothing except the palette calls them.forgeStore.refresh()andforgeStore.snapshot()are the only two remaining uses in App. Both are inside handlers and both can be reached with a dynamic import.That is roughly two hundred lines across four files, and it changes the palette's contract and its tests, which is why it is not here. The prize is seventy eight kilobytes gzipped, measured rather than estimated.
The two changes that bought nothing today are kept, because they are correct on their own terms and they are where that work starts. Reading a cut off a document should not require the drawing engine, and neither should having
toTypefaceready for a button nobody has pressed.A race in two grid tests
The full suite failed once in two hundred and forty three on the first commit. Both grid tests in
handles.spec.tsdo this:[data-forge-cells]is the grid's editor, and it appears before the alphabet has been redrawn out of cells. Read at that moment,laidwas sometimes still the skeleton. Clearing the grid puts the letter back to its skeleton, so the poll on the next line, which waits for the outline to stop beinglaid, was waiting for a change that had already happened. It could only time out.The comment above that poll already said "It went flaky on CI before it went red". That was the right observation attached to the wrong line. The poll was never the problem. The read before it was.
Both tests now take the letter before the switch is thrown and wait for it to stop being that, which is what
laidwas always supposed to mean.It passed on its own, and it passed with its own file run in parallel. Only the whole suite was slow enough to lose. Deferring the palette shifted the timing of the first screen enough to expose it, which is the honest reason it appeared now, rather than a reason to think it was new.
Checks
🤖 Generated with Claude Code
https://claude.ai/code/session_01ULPrwXh4M5YPRkd54xJKLM
Generated by Claude Code