fix(ui): supply the root-height chain AppFrame's height:100% needs#23
Merged
Conversation
AppFrame fills its host pane with height:100%, which only resolves against a definite-height ancestor chain (#root -> body -> html). The SDK shipped no such chain, so a bare app iframe resolved height:100% against a content-height ancestor and the whole shell collapsed to content height: full width, short height (issue #22). The precondition the shell depended on was neither supplied nor enforced, so multiple apps hit it. AppFrame now establishes the chain itself on render via injectBaseReset(), so every app built on it fills the pane with no per-app index.html requirement. The same reset ships as a `@nimblebrain/synapse/ui/base` side-effect import for apps that want the chain before first paint (no layout jump) or that render a full-pane root without AppFrame. The reset is a percentage chain (html, body, #root { height: 100% } plus body { margin: 0 }), not a viewport unit: percentages resolve against the actual allocated pane, staying correct on hosts that give an app a pane shorter than the viewport, where a vh/dvh unit would overflow with a second scrollbar. AppFrame's height:100% and internal scroll model are unchanged.
The 0.11.0 entry was written by replacing the 0.10.2 heading, which dropped 0.10.2 from the changelog and orphaned its (already-shipped) dark-mode-token notes under 0.11.0 — leaving a malformed section with two ### Fixed blocks. Re-add the 0.10.2 section (verbatim to the v0.10.2 tag, which is published as npm latest) above a now self-contained 0.11.0 section.
`/vh/` already matches inside `dvh`, so the alternation was redundant; the comment now records that matching "vh" catches "dvh" too.
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.
Closes #22.
The bug
AppFramefills its host pane withheight: 100%, but that only resolves against a definite-height ancestor chain (#root→body→html) — and the SDK shipped no such chain anywhere. A Synapse app iframe is its own bare document, so unless an app hand-addedhtml, body, #root { height: 100% }to itsindex.html,AppFrame'sheight: 100%resolved against a content-height ancestor and the whole shell collapsed to content height: full width, short height (e.g. an empty board rendering as a ~442px band inside an 868px pane). A precondition the shell depends on but neither supplies nor enforces is a framework defect — it bit more than one app.It went unnoticed because the gallery wraps every
AppFramein aFramewith a definiteheight: 380, so the SDK never exercised AppFrame-as-pane-root.The fix
Supply the missing precondition rather than change the height model:
@nimblebrain/synapse/ui/base— a side-effect import that injectshtml, body, #root { height: 100% }+body { margin: 0 }via the existingensureStylehelper. Import it in an app entry to establish the chain before first paint.AppFramecalls the same reset on render (injectBaseReset(), render-body, matching the components'ensureStylepattern), so every app built on it fills the pane at runtime with no per-appindex.htmlrequirement — existing apps included. The reset module is imported as a function, not a side-effect, so theuibarrel still injects nothing on import; only renderingAppFrame(or the explicitui/baseimport) applies it.AppFrame'sheight: 100%,minHeight: 0, and internal scroll model are unchanged.Why a percentage chain, not a viewport unit
The issue originally proposed
minHeight: 100dvhon the component. An adversarial review surfaced two shipping counter-signals against viewport units here:synapse-collateralalready injectshtml, body, #root { height: 100% }(the exact percentage chain).synapse-astro-editorcarries a production comment rejectingvh/dvhbecause the platform can allocate a pane shorter than the viewport, where a viewport unit overflows with a second scrollbar.Percentages resolve against the actual allocated pane on every embedding model (iframe or host panel); a viewport unit assumes pane == viewport, which isn't guaranteed. So both the reporter's
minHeight: 100dvhfloor and aheight: 100dvhwere rejected in favor of supplying the percentage chain. A min-height floor also wouldn't deliver the shell's pinned-header / internally-scrolling-body contract under overflow — a definite-height chain does.Tests
src/__tests__/ui/base.test.ts—injectBaseReset()injects the chain + margin reset, is idempotent, and is asserted to contain novh/dvh(guards against a regression back to viewport units).src/__tests__/ui/layouts.test.tsx— renderingAppFrameestablishes the root-height chain (the direct regression guard for AppFrame uses height:100% with no root-height guarantee — apps silently collapse to content height in the host iframe #22).npm run ci(lint → typecheck → build → test) green; 389 tests pass. Version bumped to 0.11.0 (additiveui/baseexport) with a CHANGELOG entry, so it's release-ready on merge.