Do not take the connection down over a default layer - #54
Merged
Conversation
`applyThemeVariables` guarded `typeof document === "undefined"`, which asks whether a document exists, not whether it can carry a stylesheet. Those were the same question while the defaults were inline properties on `documentElement` — `style.setProperty` is all a partial document has ever needed to provide. Putting them in a `<style>` element added `getElementById`, `createElement` and `head.prepend`, and an embedder or test harness that shims only `documentElement.style` and listeners has none of them. The throw did not stay local. It unwound through `applyTheme` into the handshake, so an app in such an environment never finished connecting: the symptom is a dead session, and nothing in it points at theming. A default layer is a rendering nicety and a session is not, so a document that cannot host a stylesheet now gets no layer and keeps its connection. `removeProperty` gets the same treatment, and is the more dangerous of the two: `appliedInlineKeys` is empty on the first apply, so the clear pass is skipped entirely and the throw waits for a theme toggle. The second test covers exactly that ordering — without it a single-apply test passes against a `style` carrying only the setter. Found by the NimbleBrain host's SDK-parity suite, whose document stub is the shape both new tests mirror.
The first pass fixed the `createSynapse` path and wrote a CHANGELOG entry claiming a document that cannot carry a stylesheet keeps its session. The SDK has three connection entry points and the other two still died on the same document, so the entry described an invariant that did not hold. `connectUI` → `applyHostTheme` sets `data-theme` via `documentElement.setAttribute`, behind exactly the `typeof document` check this change exists to argue against — and one line ahead of the stylesheet install already known to fail there. It is the vendored `window.SynapseUI` IIFE, so the asset this branch rebuilds shipped the throw. `connect` → `createResizer(...).measureAndSend()` reads `document.body.scrollWidth` with no guard at all, at step 2, before `ui/initialize` goes out. `connect` is async, so it surfaces as an unhandled rejection — the quiet failure, which is the same argument already made for the `removeProperty` check. Both are feature-checked at the point of use now, which is what `applyThemeFontFaces` has always done with `document.fonts` — the pattern was already house style in this file, just applied unevenly. A document that cannot take an attribute still gets its tokens; one with no body reports no size; the session survives either way. The CHANGELOG no longer generalises from one demonstrated caller. It names the three paths and what each degrades to, and drops the appeal to unspecified embedders — the only partial document anyone has produced is a test stub, and the case for the guard is that a decoration must not cost a connection, which holds regardless of who the caller is.
The previous round extended the guards to `applyHostTheme`'s `setAttribute` and the resizer's `document.body` on the argument that the invariant should hold across all three entry points. Checking when each site actually broke undoes that argument: both were unguarded in 0.12.2 and 0.13.0. They are not regressions, and a partial-DOM caller on `connect` or `connectUI` was already failing before 0.14.0 existed. Guarding them here restores nothing — it newly promises a capability this package has never had. And the promise does not close. That pass still left `resize.ts:47` (`observer.observe(document.body)`, in the file it was editing), `mcpapps.ts:153` and `mcpapps.ts:226` — the last two on the Claude and NimbleBrain path, which is the one that matters most. Point-of-use feature-checking is not an invariant; it is a list that grows every time someone looks, and a half-delivered guarantee is worse than none because the gaps read as deliberate. What 0.14.0 actually broke is precise and small: through 0.13.0 the theming path needed `documentElement.style.setProperty` and a feature-checked `document.fonts`, and 0.14.0 added `getElementById`, `createElement`, `head.prepend` and `removeProperty`. Guarding exactly those restores the prior envelope and nothing else, which is what a patch should do. Whether this SDK should run against a document that is not a browser's is a real question, but it is a design decision with a capability envelope and a test harness behind it, not four lines in a regression fix. The only partial document anyone has produced is a stub in the host's own parity suite that does not model a browser; completing it is the smaller and more honest fix, and belongs with that suite.
`applyThemeVariables` also feature-checked `documentElement` and `setProperty`. Neither is a 0.14.0 requirement: 0.13.0 read `document.documentElement.style` and called `setProperty` unguarded, so a document without them was already unsupported. Checking them restores nothing and newly promises a capability this package has never had — the argument the previous commit used to drop the `applyHostTheme` and `document.body` guards, applied to the file it was editing. It was also unreachable from any caller: the full suite passes with those two lines removed, and the host stub this branch was written against supplies `setProperty`. What it bought instead was a silent return on the one path that writes the host's variables at all. `canInstallStylesheet` and the `removeProperty` check stay. Those are the regression, and with the over-guard gone the CHANGELOG's claim to restore 0.13.0's envelope exactly is true as written.
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
applyThemeVariablesguardstypeof document === "undefined". That asks whether a document exists, not whether it can carry a stylesheet — and those were the same question right up until 0.14.0.While the defaults were inline properties on
documentElement,style.setPropertywas the only DOM capability this module needed, and a partialdocumentshim provides it. 0.14.0 moved them into a<style>element, which added three requirements —getElementById,createElement,head.prepend— with no guard covering them. An embedder or test harness that shimsdocumentElement.styleplus listeners and nothing else now throws:The throw is not contained. It unwinds through
applyThemeinto the handshake, so an app in that environment never finishes connecting. The symptom is a dead session, and nothing about it points at theming.The fix
A default layer is a rendering nicety; a session is not. A document that cannot host a stylesheet gets no layer and keeps its connection. The host's own variables are still written inline — the part that actually carries its brand.
removePropertyis feature-checked for the same reason, and it is the quieter of the two:appliedInlineKeysis empty on the first apply, so the clear pass is skipped and the throw waits for a theme toggle rather than firing at connect.setPropertyis deliberately not checked — it is the one capability this module has always required, so a document lacking it was never supported and gets no new promise here. Guarding exactly the four capabilities 0.14.0 added restores the prior envelope and nothing else.Verification
npm run cigreen — 36 files, 498 tests (up 2).Two new tests, mutation-checked:
canInstallStylesheetguardremovePropertycheckThat second test exists precisely because a single-apply test passes against a
stylecarrying only the setter — the ordering is the whole point.How it was found
The NimbleBrain host's
test/unit/sdk-envelope-parity.test.tswent red on the 0.14.0 bump (NimbleBrainInc/nimblebrain#804) with 5 failures, all downstream of this throw. Itsdocumentstub is the shape both new tests mirror.Worth naming: this was raised in review of #50 as a suggestion — that
applyDefaultThemeLayerreachesdocument.headbehind only anundefinedcheck — and declined as unreachable from a real caller. The reasoning given at the time was correct and the rebuttal was not: a partial-DOM embedder is a real caller, and "the old code only toucheddocumentElement.style, which is unconditionally present" named the exact mechanism.