Repository navigation
feat(browser): Angular integration (@maple-dev/browser/angular, /angular/server) - #1133
JeremyFunk wants to merge 2 commits into
Conversation
@maple-dev/browser/angular: provideMapleTracing() subscribes to the router events from an environment initializer (browser only) and names each span after the matched routes' paths; tracedResolver() excludes a thrown RedirectCommand; MapleErrorHandler and reportAngularError() report what Angular's ErrorHandler sees. No decorators, so no compiler or linker pass is needed. @maple-dev/browser/angular/server: tracedRender() runs the @angular/ssr render in an ssr span and adds the Server-Timing header the page load joins.
- End spans outside Angular's zone, so a zone.js app still becomes stable after a navigation instead of waiting for the exporter's timer. - reportAngularError skips the errors Angular's global listeners build from an error event without an error object (cross-origin "Script error."), which the SDK's own window handler already handled. - tracedRender reads the Server-Timing value before the render's await, so it names the ssr span even where async context is lost across await. - Docs: the example works on Angular 19, and notes withFetch() for Angular 21 and older.
|
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 Adds two Angular subpaths to
FindingsWarning · F1 ·
|
| Change | Kind | Observable | Evidence |
|---|---|---|---|
Angular router navigations (provideMapleTracing) |
client span | yes | angular/index.ts:68 starts pageload/navigate, ended at :97 by routeTemplate |
Resolver data loading (tracedResolver) |
client span | yes | angular/index.ts:122 delegates to traced (navigation.ts:91) |
Angular errors (MapleErrorHandler, reportAngularError) |
error span | yes | angular/index.ts:134 captureException(error, { name: "angular.error" }) |
Angular SSR render (tracedRender) |
server span | yes | angular/server.ts:24 startActiveSpan("ssr", { attributes }) with url.path |
Copy all findings (1)
Findings from an automated review of commit 16f3767d77b0eb92632195d9b8c9be8b3a17e5e1. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.
---
F1 · Warning · correctness · packages/browser/src/angular/index.ts:121-123
`tracedResolver` ends its span inside NgZone, defeating the zone.js fix
`provideMapleTracing` ends the navigation span outside Angular's zone (`index.ts:53`) because, as its own comment says, a span that ends in the zone starts the exporter's flush timer as a zone macrotask and keeps `ApplicationRef.isStable` false. A resolver's span is not covered: `traced` runs at `navigation.ts:91` and `runTraced` ends the span at `failures.ts:79`, both inside the zone the router runs resolvers in, so on a zone.js app every route with a resolver (or any `traced` call during the navigation) is back to waiting on the export timer after the navigation — the exact behaviour `zone.browser.test.ts:37` asserts the fix removes, tested only on a route table with no resolver. The `angular.error` span has the same problem.
Suggested fix: End resolver spans outside the zone as well: keep the `NgZone` captured in `provideMapleTracing` and run resolver span endings (or `tracedResolver`'s whole `traced` call, via `inject(NgZone, { optional: true })?.runOutsideAngular(...)`) through it. Worth one zone.js test with a resolver on the route.
16f3767 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple to ask about one.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for (let route: ActivatedRouteSnapshot | null = root; route; route = route.firstChild) { | ||
| // Layout routes and `children` wrappers have an empty path | ||
| if (route.routeConfig?.path) paths.push(route.routeConfig.path) | ||
| } |
There was a problem hiding this comment.
🟡 Secondary-outlet routes lose their names
When a navigation changes a named outlet alongside the primary outlet, routeTemplate follows only firstChild. Distinct secondary routes receive the same navigation span name.
Learn more
An Angular route snapshot can have several active children when the application uses named outlets. The current traversal reads only firstChild, so it cannot distinguish routes that differ only in another outlet. Navigation spans therefore group distinct user actions under one route template.
Example: With a primary inbox route and a sidebar outlet, /inbox(sidebar:compose) and /inbox(sidebar:settings) both produce navigate /inbox when the primary child appears first. Their templates must remain distinguishable.
Recommended fix: Traverse ActivatedRouteSnapshot.children and include the configured path for each active outlet in a stable, outlet-qualified route template. Cover a primary route plus a named outlet and a navigation that changes only the named outlet.
Was this helpful? React with 👍 or 👎 to provide feedback.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| * Run a resolver's data loading in a span under the navigation. Errors are recorded once and | ||
| * rethrown; a thrown `RedirectCommand` isn't one. Only requests started before `fn`'s first `await` nest under it. | ||
| */ | ||
| export function tracedResolver<T>(name: string, fn: () => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
tracedResolver ends its span inside NgZone, defeating the zone.js fix
F1 · Warning · correctness
provideMapleTracing ends the navigation span outside Angular's zone (index.ts:53) because, as its own comment says, a span that ends in the zone starts the exporter's flush timer as a zone macrotask and keeps ApplicationRef.isStable false. A resolver's span is not covered: traced runs at navigation.ts:91 and runTraced ends the span at failures.ts:79, both inside the zone the router runs resolvers in, so on a zone.js app every route with a resolver (or any traced call during the navigation) is back to waiting on the export timer after the navigation — the exact behaviour zone.browser.test.ts:37 asserts the fix removes, tested only on a route table with no resolver. The angular.error span has the same problem.
End resolver spans outside the zone as well: keep the `NgZone` captured in `provideMapleTracing` and run resolver span endings (or `tracedResolver`'s whole `traced` call, via `inject(NgZone, { optional: true })?.runOutsideAngular(...)`) through it. Worth one zone.js test with a resolver on the route.
Prompt for an AI agent
In `packages/browser/src/angular/index.ts:121-123`: `tracedResolver` ends its span inside NgZone, defeating the zone.js fix.
`provideMapleTracing` ends the navigation span outside Angular's zone (`index.ts:53`) because, as its own comment says, a span that ends in the zone starts the exporter's flush timer as a zone macrotask and keeps `ApplicationRef.isStable` false. A resolver's span is not covered: `traced` runs at `navigation.ts:91` and `runTraced` ends the span at `failures.ts:79`, both inside the zone the router runs resolvers in, so on a zone.js app every route with a resolver (or any `traced` call during the navigation) is back to waiting on the export timer after the navigation — the exact behaviour `zone.browser.test.ts:37` asserts the fix removes, tested only on a route table with no resolver. The `angular.error` span has the same problem.
Suggested fix: End resolver spans outside the zone as well: keep the `NgZone` captured in `provideMapleTracing` and run resolver span endings (or `tracedResolver`'s whole `traced` call, via `inject(NgZone, { optional: true })?.runOutsideAngular(...)`) through it. Worth one zone.js test with a resolver on the route.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
This branch has not been deployed
1 participant
Summary
An Angular integration for
@maple-dev/browser, replacing the router, resolver,ErrorHandlerand SSR glue the Angular frontend guide had customers copy. Stacked on #1128 (/server,serverTiming(), the Next.js entries).@maple-dev/browser/angular→provideMapleTracing(): EnvironmentProvidersprovideRouter: apageload/navigatespan per navigation, named after the matched routes' paths (/projects/:id,/**). Nothing on the server@maple-dev/browser/angular→tracedResolver(name, fn)RedirectCommandisn't an error@maple-dev/browser/angular→MapleErrorHandler{ provide: ErrorHandler, useClass: MapleErrorHandler }: reports asangular.error, then logs like Angular's own handler@maple-dev/browser/angular→reportAngularError(error)ErrorHandler,onViewError, or a redirectingwithNavigationErrorHandler@maple-dev/browser/angular/server→tracedRender(req, render)angularApp.handle(req)inserver.ts: anssrspan under the request span, plus theServer-Timingheader the page load joinsThe entry is plain ESM: functions and an undecorated class, no
ɵɵngDeclare*, so no compiler or linker pass.MapleErrorHandlerworks withuseClassthrough Angular's zero-argument-constructor path./angular/serverimports only@opentelemetry/api.@angular/core,@angular/routerand@angular/commonare optional peers (*); the entry needs Angular 19+ (provideEnvironmentInitializer). The main entry's bundle is unchanged: eager 37.42 kB, first-party 14.30 kB.Behaviour notes (vs the guide's code)
withDebugTracing, not an app initializer. It runs before every app initializer, so it also sees a navigation that one of them starts. The guide'sprovideAppInitializerversion misses it when that initializer is listed first (test covers it).NavigationSkipped, not aNavigationStart. The guide leftredirectingset, so the user's next click started no span. That span now ends named after the route on screen, and the flag resets.app.navigation.interrupted, like any replaced navigation. The guide ended it unmarked.ApplicationRef.isStablefalse. This also affected the guide's code (test with zone.js covers it).reportAngularErrorskips theErrorthatprovideBrowserGlobalErrorListeners()builds from anerrorevent with no error object (causeis theErrorEvent, such as a cross-origin "Script error."). The SDK's own window handler already handled that event.tracedRenderreads theServer-Timingvalue before awaiting the render, so it names thessrspan even when async context doesn't surviveawait(zone.js on the server). It records a render error on the span and rethrows it. It leaves headers it can't change (immutable) alone. The span ends whenhandle()resolves, before the response is written.Verification
packages/browser: typecheck, 175 tests, build, size budget. The Angular tests use the real router (createApplication,provideLocationMocks, JIT compiler in Chromium) and one realbootstrapApplication. Cases: naming, nested and layout routes,**, query and hash, guard redirect, redirect to the URL on screen, resolverRedirectCommandreturned and thrown, superseded navigations, same-URL supersede, guard rejection, resolver error recorded once throughMapleErrorHandler, back/forward, initializer-started navigation, double provide, server platform, "Script error.", and zone.js stability. Server tests cover the Node request shape and fetchRequestURLs, lost async context, immutable headers and errors. Every fix above was mutation-checked: its test fails without it.ng build,node --import ./telemetry.mjs dist/.../server.mjs), then driven with the same Playwright script (22 scenarios).ng buildconsumed the entry without linker errors./projects/1GET→ssr→ loader + 2 fetch → API;pageload /projects/:idunderssr; reload gives a new trace/old(302),/slow,?tab=memberspageload /projects/:id,/slow(1.5 s),/projects/:id/projects/missing,/does-not-existpageload /not-found,pageload /**, no error spans/broken-loader/broken-renderangular.errorinssrtrace + once on hydrationnavigate /projects/:id→ loader → fetch → APInavigate /projects/:id,navigate //oldnavigate /projects/:id,url.path=/old/slowthen/projects/1)navigateINTERRUPTED +navigate /projects/:idnavigate, not markednavigateINTERRUPTEDnavigate /,navigate /projects/:idnavigate /not-found,navigate /**angular.erroronce;angular.erroronce/api/boomtraceparent, third party noneserver-timingon JS/CSSKnown limits
matcherfunction have nopath, so they add nothing to the span name.url.pathis the router's URL: it excludes the<base href>and keeps matrix params.angular.errorspan from theErrorHandler.tracedRenderopens anssrspan for requests Angular doesn't render (handle()resolvesnull), as the guide's middleware did.tracedRender, or every visitor joins one trace.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.