fix: full reload when SPA navigation crosses an app boundary#119
fix: full reload when SPA navigation crosses an app boundary#119
Conversation
LinkInterceptor optimistically swaps body content while keeping the head intact when navigating between same-origin pages. That works for intra-app navigation (different routes within one LiveTemplate handler) but produces silently-broken renders when the destination is a different app entirely — the docs body lands in a page whose head still references the previous app's stylesheets. Reproduced live on livetemplate.fly.dev: the docs site reverse-proxies a separately-deployed pattern showcase at /patterns/*. Clicking the "Patterns" link in the proxied app's header (href="/") fetches the docs root, swaps the body, and leaves the head pointing at the showcase's pico.min.css + livetemplate.css. The result is the docs HTML rendered with the wrong stylesheet — sidebar collapses to inline flow, layout grid breaks, "fixed by reload" because reload pulls a fresh head. handleNavigationResponse now compares the set of <link rel="stylesheet"> hrefs in the fetched HTML against those in the live document. If they differ — the heuristic that catches a cross-app boundary — fall through to window.location.reload() so the browser performs a real navigation that loads the complete document. Same-stylesheet navigation continues on the optimized swap path. Tests cover both directions: cross-app stylesheets ⇒ body swap path short-circuited (disconnect not called, wrapper content preserved); matching stylesheets ⇒ swap path proceeds normally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Good fix for a real bug. The heuristic is well-reasoned and the test coverage is solid. A few actionable issues: 1.
2. In A cleaner signature: 3. The test comment is honest about the jsdom limitation. The current test only verifies the early-return (disconnect not called), not that a reload is actually triggered. Consider extracting the reload into a thin wrapper (e.g. 4. For links already parsed into the live Overall: the logic is correct, the tests cover the important paths, and the PR description is excellent. Issues 1–2 are worth addressing before merge; 3 is nice-to-have; 4 is a nitpick. |
- Replace window.location.reload() with explicit window.location.href
via a private performFullNavigation(destinationHref) wrapper. The
previous code worked by relying on link-interceptor's pushState
having already updated window.location to the destination — that
ordering invariant wasn't obvious and would silently navigate to
the wrong URL if it ever changed. The wrapper also makes the path
spy-able so the cross-app test can assert the destination.
- Thread destinationHref through handleNavigationResponse and into
stylesheetsDiffer, then use it as the base for resolving the
fetched doc's relative stylesheet URLs. Previously the live doc's
base was used for both, which is correct only because production
apps use absolute or root-relative stylesheet URLs.
- For the live doc's stylesheets, use HTMLLinkElement.href directly
instead of getAttribute("href") + URL resolution. The browser-
resolved value is already absolute and avoids the try/catch.
The cross-app boundary test now asserts performFullNavigation was
called with the exact destination URL, closing the loop on what was
previously an untestable reload() side effect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Good fix for a real problem. A few things worth addressing: Bug in PR description Breaking interface change
current.forEach((href) => {
if (!fetched.has(href)) differs = true;
});
for (const href of current) {
if (!fetched.has(href)) return true;
}
return false;Potential false positive: Overall: Logic is sound, the |
Pinning to the exact version that ships livetemplate/client#119 (full reload when SPA navigation crosses an app boundary). The patterns app loads via the docs site's reverse proxy, so its client library was the one intercepting the cross-app link click and patching the docs body into a page whose head still pointed at the patterns app's stylesheets. Pinning (vs. @latest) is intentional here so the fix is live the moment this branch deploys, instead of waiting for jsdelivr's edge cache on @latest to rotate. Once the dust settles we can revert to @latest in a follow-up to keep getting client patches automatically. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #119 placed the cross-app stylesheet check inside the if (newWrapper) branch of handleNavigationResponse, where newWrapper is the [data-lvt-id] element parsed from the fetched HTML. When the destination is a static page that has no [data-lvt-id] anywhere — like a tinkerdown-rendered docs root — querySelector returns null, the check is skipped, and execution falls through to the no-wrapper body-replace fallback at the bottom of the function. That fallback swaps body content unconditionally, leaving the previous app's <link rel="stylesheet"> tags in place: the exact head-vs-body mismatch the guard is supposed to prevent. Real-world repro: clicking the Patterns header link (href="/") from the LiveTemplate patterns showcase navigates to the docs site root. The patterns app loads @livetemplate/client, so LinkInterceptor fetches /, finds no [data-lvt-id] in the response, skips the cross-app guard, and swaps the docs body into a page whose head still references the patterns app's pico.min.css and livetemplate.css. Move the check before the newWrapper branch so it runs regardless of which body-swap path the response would take. Adds a new test case ("triggers a full navigation even when destination has NO data-lvt-id") that pins this case directly. Also caught when the manual chromedp repro after deploying v0.8.41 still showed the broken state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
LinkInterceptor's SPA optimization patches the body and updates<title>while leaving<head>alone. That's correct for navigating between routes within one app, but breaks when the destination is a different app whose<head>declares different stylesheets — the new body ends up styled by the previous app's CSS.handleNavigationResponsenow compares the set of<link rel="stylesheet">URLs in the fetched HTML to those in the live document. Different sets ⇒window.location.reload()to force a real navigation. Same sets ⇒ proceed on the swap path as before.How this was reproduced
Live on
livetemplate.fly.dev(the LiveTemplate docs site, which reverse-proxieslt-patterns.fly.devat/patterns/*):/patterns/forms/click-to-edit(proxied content from the patterns app)href=\"/\"/, swaps the body<head>still has the patterns app'spico.min.css+livetemplate.css; the docs site'spico.css+tinkerdown-client.css+prism.cssare goneReproduced in chromedp:
Test plan
cross-app boundarydescribe block intests/navigation.test.ts:disconnectnot called, wrapper content preserved — guarantees no half-swapped state)Why this heuristic
Stylesheet-href set is a noisy-but-effective proxy for "different
<head>":/assets/)<head>template, so the set is identicalOther signals considered and rejected:
<head>child node ⇒ too noisy (transient<style>blocks added by LiveTemplate)<title>⇒ no signal; titles change within an appNotes
This is an additive guard. The optimization path is unchanged for any navigation where it's actually safe.
🤖 Generated with Claude Code