fix(editor): make the embedded surface fill its host container instead of the viewport - #23
Merged
Merged
Conversation
…d of the viewport The host surface sized itself with h-screen (100vh) all the way down, but an embedded consumer hands it a container that is the viewport minus its own app chrome. Inside CodePress's h-[calc(100dvh-73px)] main, the surface overflowed by exactly the header height and the host's overflow-y-auto turned that into the scrollbar nobody asked for. LoadedEditor is shared between both modes, so it now fills its parent (h-full) and the standalone entry point supplies the viewport instead. The surface, its loading placeholder and its error placeholder are all height:100% - no viewport unit anywhere, and no knowledge of any host's chrome. Pre-commit hook skipped: it runs vp check --fix over staged files, and packages/freecut-editor/consumer-smoke.test.tsx cannot resolve @quantfive/freecut-editor-surface in this repo (pre-existing, present at base).
…pshot The surface only subscribes to the controller inside mountStores(), which the editor tree calls from a passive effect. Waiting on the editor node appearing in the DOM does not guarantee that effect has flushed, so a snapshot pushed right after could be dropped and the assertion failed intermittently under load. Wait on the store lifetime itself instead. Pre-commit hook skipped for the same pre-existing consumer-smoke resolution error as the previous commit.
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.
The bug
The embedded editor surface sized itself to the viewport (
h-screen/min-h-screen) while the host renders it inside a container that is viewport-height minus the app header. The surface therefore always overflowed by exactly the header height, forcing the host page to scroll.Measured, not inferred
Reproduced in a fixture matching the host layout:
clientHeightscrollHeightOverflow was exactly 73px — the header height. That confirms the diagnosis rather than assuming it.
Four elements, not one
The loaded surface was the obvious one. The error and loading placeholders (
editor-surface.tsx) are what occupies the container beforehost.load()settles — fixing only the loaded path would still overflow during load, which is the first thing a user sees.editor-surface.tsx— wrapperh-screen→h-full; error + loading placeholdersmin-h-screen→h-full min-h-0editor.tsx—h-screen→h-fullStandalone FreeCut is unchanged
Standalone is the regression risk: it has no host container, so it should still take the viewport.
Editornow supplies that itself via adata-freecut-editor-shell="standalone"wrapper carryingh-screen min-h-0.Verified against a separate pre-fix build of the same commit in headless Chromium, driven through the real workspace gate: byte-identical geometry both ways.
Rejected approach
Global height rules in
index.csswould have worked — and would ship inside the published package, imposing those rules on every consumer's page from a file they did not know they inherited. Kept scoped to the components instead.Test race
The new coverage made a pre-existing race in
editor.test.tsxstart flaking. Root cause: the test waited on a DOM node as a proxy for a passive effect having flushed. Fixed the cause (wait for the host runtime's stores) rather than adding a retry —d059e48d.Acceptance
Ships in 0.3.7 alongside the audio fix (#22).