Repository navigation
feat(browser): Astro integration - #1134
JeremyFunk wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Maple reviewConfidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Adds the
|
| const HTML_TAG = /^<html(?=[\s>])[^>]*>/i | ||
| const HAS_ROUTE = /\sdata-route[\s=>]/i |
There was a problem hiding this comment.
🟡 Quoted attributes corrupt route stamping
When an HTML attribute contains > before an existing route, HTML_TAG misses that route and inserts a duplicate. When attribute text contains data-route=, HAS_ROUTE suppresses stamping entirely. Page spans then use the wrong route or keep generic names.
Learn more
The route stamper scans the opening <html> tag before adding the route template used to name browser spans. Its regular expression treats the first > as the end of the tag even when that character appears inside a quoted attribute. Its route check also searches raw tag text rather than parsed attributes, so quoted attribute values can impersonate data-route. Both cases make the emitted markup disagree with the intended route selection.
Example: With <html data-note="a > b" data-route="/custom">, the matcher stops at the quoted > and inserts a second data-route before /custom. With <html data-note=" data-route=/fake">, it sees no actual route attribute but adds none.
Recommended fix: Parse the <html> opening tag with a quote-aware scanner, including the lookahead across stream chunks. Check only actual attribute names for data-route, then insert the route only when it is absent.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
An Astro integration for
@maple-dev/browser, replacing the glue the Astro frontend guide has customers copy (tracing.ts, the layout script,data-route={Astro.routePattern},src/middleware.ts). Stacked on #1128.Plus the customer's own
MapleBrowser.init()script in the base layout. Nothing else goes in the layouts.@maple-dev/browser/astro→maple(): AstroIntegration(default)addMiddleware(orderpre) +injectScript("page", …): wires the two entries below into every page@maple-dev/browser/astro/client→traceAstroNavigation(): void<ClientRouter />navigations,astro:hydration-errorreporting. The integration calls it; without the integration, call it from the layout script@maple-dev/browser/astro/middleware→onRequest: MiddlewareHandlerdata-routeon each page's<html>(at build time for prerendered pages),ssr <route>span +Server-Timingfor on-demand pages. Composes withsequence()astrois an optional peer (*), a dev dep for types and tests (same 7.3.5 asapps/landing)./astro/middlewaredepends on@opentelemetry/apionly. The main entry is unchanged: eager 37.42 kB, first-party 14.30 kB, same as the base branch.Why an integration
Astro.routePatternonto the<html>start tag as the HTML streams past. Only the bytes up to the end of that tag are held back. Prerendered pages get it at build time: Astro runs middleware for them and writes the response it returns. A page that already hasdata-route(the guide's setup) keeps its own, and fragments (server islands, partials) pass through. After a rewrite (Astro.rewrite), the page that rendered wins, because Astro runs the middleware again for it.injectScript("page")is one module, once per page, and<ClientRouter />doesn't run it again after a swap. This removes the guide's "two layouts, two scripts, the second ends the first as interrupted" pitfall.traceAstroNavigationis also idempotent.initstays in user code. Passing init options through the integration isn't safe.privacy.sanitizeUrlis a function, which can't be serialized into the injected script.import.meta.env.PUBLIC_*isn't whatastro.config.mjssees. The integration's script can land before the user's (it renders at the end of<head>), sotraceAstroNavigationstarts the page load atDOMContentLoaded, after every module script,initincluded, has run.Behaviour vs the guide
Same span names, parents and edge cases:
pageload <route>per document ended atload;navigate <route>fromastro:before-preparationtoastro:page-load, with aload pagespan and the page request under it; an aborted navigation isnavigateinterrupted; a full-page-load fallback isnavigatewith the generic name; redirects are named after the destination; hash links start no span;astro.hydration_errorfor island code that fails to load;ssr <routePattern>on the server, ending when streaming starts, withServer-Timingon HTML only, skipping prerendered pages and route-cached ones.Changes:
loadevent. The guide ended the in-flightnavigatespan atload, and at theastro:page-loadthe router fires for that sameload, named after the old page. E2E:navigate /heavy608 ms (old) vsnavigate /slow-server1542 ms (new). Now theloadlistener only ends the document's page load, andastro:page-loadonly ends a navigation whose page was swapped in.astro:page-load. The page load of a swap that a newer navigation overtook no longer ends the newer one.Server-Timingis skipped on more cacheable responses, since the customer can't edit packaged middleware: Astro's route cache (maxAgeorswr),Cache-Controlwithpublic,s-maxageormax-age> 0,CDN-Cache-Controland vendor variants,Surrogate-Control(all unlessno-store/private).fetch()response with immutable headers doesn't throw.Set-Cookieand the status are kept.content-lengthis dropped when the body changes, and compressed bodies aren't read.Verification
packages/browser: typecheck, 182 tests (node + Chromium), build, size budget. New tests:html.test.ts: tag lookup, chunk splits, multibyte text, escaping, lookahead cap.middleware.test.ts: the realsequence()fromastro/middleware, streaming, rewrite re-run, cache rules, errors, Astro 4 and 6 contexts.client.browser.test.ts: Astro's realdoPreparation/TransitionBeforeSwapEvent/onPageLoadfromastro/virtual-modules/transitions-events.js: interrupted, fallback,loadrace, stale page-load, DOMContentLoaded ordering, idempotency, hydration error.oxlint+oxfmton changed files;astro checkon the e2e app.@astrojs/node, prerendered pages plusprerender = falsepages, Node SDK preloaded), switched to the packed tarball withintegrations: [maple()]and only theinitscript left. Production build +node --import, Playwright over 21 scenarios, both builds (MPA andPUBLIC_CLIENT_ROUTER=1). Spans match the old run:url.pathconcretessr /server, new trace on reload, no header on prerendered/assets/404/redirectssrspan Error + exception, no headernavigate /projects/[id]>load page>GET> server/oldredirect/404template), no error spansastro.hydration_errortraceparent, third party none(
=means the normalized span tree is identical. The only other differences: one third-party and one slowfetchspan that the old run dropped at page exit, and probe noise.)Extra scenarios on the new build:
loadrace: fixed (above).Astro.rewrite("/server"):data-route="/server",ssr /rewritten>ssr /server>pageload /server.ssr /_server-islands/[name].Set-Cookiekept.astro preview: H1/H3–H9 correct.astro dev: stamped and traced.export { onRequest }viasequence(maple, own)+traceAstroNavigation()): H2, H3, H7b, rewrite,loadrace all correct, and the user middleware's header is kept.Known limits
routePattern,isPrerendered). On Astro 4 the middleware passes everything through.astro:hydration-errorneeds 6.3, and the route-cache check needs 7.ssrspan ends when streaming starts, as in the guide. Components rendered after that nest under it but run past its end, and an error thrown mid-stream isn't recorded.<html>(no doctype/comment preamble then<html>within 16 KiB) gets no route, and its spans keep the generic name.initscript. To trace only some pages, wire it by hand.Server-Timingtraceparents (inner first). The browser joins the innerssrspan, in the same trace.astro:before-preparationlistener cancels, or whose custom loader throws, ends as interrupted at the next navigation or page exit (as in the guide).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.