feat(ember)!: Update to v2 addon format - #23252
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const globalsPerformanceConfig = { | ||
| disableRunloopPerformance: options.disableRunloopPerformance ?? false, | ||
| minimumRunloopQueueDuration: options.minimumRunloopQueueDuration, | ||
| minimumRunloopQueueDuration: options.minimumRunloopQueueDuration ?? 0, |
There was a problem hiding this comment.
Idle timeout default dropped
Medium Severity
The Ember integration no longer sets idleTimeout to 5000. It now inherits the browser default of 1000ms. Quiet stretches during a transition (for example a slow model hook without child spans) can finish the navigation/pageload span early, truncating Ember route timing that previously waited up to 5 seconds.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1f20412. Configure here.
4b990e8 to
4dcd6de
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4dcd6de. Configure here.
| appInstance, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
FastBoot guard removed despite docs
Medium Severity
The v1 initializer skipped instrumentation when service:fastboot reported FastBoot, but the new instrumentAppInstancePerformance path has no equivalent check. UPGRADE.md still claims FastBoot is detected automatically, so SSR apps can run browser performance setup on the server.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4dcd6de. Configure here.
| return { | ||
| [URL_PATH]: path, | ||
| [URL_FULL]: getAbsoluteUrl(fullUrl), | ||
| [URL_FULL]: filterCollectedUrl(getAbsoluteUrl(fullUrl)), |
There was a problem hiding this comment.
URL filter omits in-scope client
Low Severity
Flagged because of the project review rule on unfiltered URL attributes. filterCollectedUrl() is used for url.full, but the in-scope client from instrumentEmberAppInstanceForPerformance is not passed through, so filtering can fall back to the wrong client.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 4dcd6de. Configure here.
size-limit report 📦
|
4dcd6de to
6884073
Compare
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
|
|
||
| "@babel/plugin-transform-runtime@7.18.10", "@babel/plugin-transform-runtime@^7.13.9": | ||
| "@babel/plugin-transform-runtime@7.18.10": |
There was a problem hiding this comment.
Critical severity vulnerability may affect your project—review required:
Line 2594 lists a dependency (@babel/plugin-transform-runtime) with a known Critical severity vulnerability.
ℹ️ Why this matters
Affected versions of @babel/traverse and babel-traverse are vulnerable to Incomplete List of Disallowed Inputs / Incorrect Comparison. Compiling untrusted code with Babel using plugins that invoke the internal path.evaluate() or path.evaluateTruthy() methods (for example @babel/plugin-transform-runtime, @babel/preset-env with useBuiltIns, or any polyfill‐provider plugin) allows a maliciously crafted AST to execute arbitrary code on the build machine during compilation.
To resolve this comment:
Check if you use Babel to compile untrusted JavaScript.
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| transitionSpan = startInactiveSpan({ | ||
| attributes: { | ||
| // TODO(conventions): Replace `'router'` with the `router` span op constant once it is released in `@sentry/conventions`. | ||
| [SENTRY_OP]: 'router', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember', | ||
| }, | ||
| name: `route:${fromRoute} -> route:${toRoute}`, |
There was a problem hiding this comment.
Bug: A transitionSpan is incorrectly created on initial page load when fromRoute is null, leading to a malformed span name and potential attribute overwrites on the pageload span.
Severity: MEDIUM
Suggested Fix
Add a conditional check to ensure transitionSpan is not created when fromRoute is null. This will restrict transition span creation to actual route-to-route navigations and prevent instrumentation interference during the initial page load.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/ember/src/utils/instrumentEmberAppInstanceForPerformance.ts#L134-L140
Potential issue: During an initial page load, the `routeWillChange` event fires with a
`null` value for `fromRoute`. The code unconditionally creates a `transitionSpan` for
this event, resulting in a span with a semantically incorrect name like `"route:null ->
route:InitialRoute"`. This span is created as a child of the main pageload span
(`activeRootSpan`). When the subsequent `routeDidChange` event fires, it ends this
incorrect span and modifies attributes on the parent `activeRootSpan`, which can
overwrite important pageload data.
Did we get this right? 👍 / 👎 to inform future reviews.
| afterAllSetup(client) { | ||
| integration.afterAllSetup(client); | ||
|
|
||
| // Run this in the next tick to ensure the ember router etc. is properly initialized | ||
| instrumentEmberAppInstanceForPerformance( | ||
| client, | ||
| appInstance, | ||
| appInstancePerformanceConfig, | ||
| startBrowserTracingPageLoadSpan, | ||
| startBrowserTracingNavigationSpan, | ||
| ); |
There was a problem hiding this comment.
Bug: The removal of the _hasMountedSentryPerformanceRouting guard allows instrumentEmberAppInstanceForPerformance to be called multiple times, which will register duplicate router event listeners.
Severity: MEDIUM
Suggested Fix
Reintroduce a guard to prevent instrumentEmberAppInstanceForPerformance from executing more than once for the same application instance. This can be achieved by setting a flag on the appInstance or within a module-level scope after the first execution.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/ember/src/utils/browserTracingIntegration.ts#L54-L63
Potential issue: The removal of the `_hasMountedSentryPerformanceRouting` guard, which
previously prevented re-instrumentation, introduces a regression. While the Sentry
integration system deduplicates integrations by name, it is still possible to call
`instrumentEmberAppInstanceForPerformance` directly multiple times for the same
application instance. This will bypass the integration-level check and attach duplicate
event listeners to the Ember router service, leading to redundant instrumentation and
potential unpredictable behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8a593fd to
01de976
Compare
1 participant
Rebased continuation of #19229 (original author @aklkv) onto current
develop, with merge conflicts resolved and the failing tests fixed.Migrates
@sentry/emberfrom the legacy v1 addon format to the Ember v2 addon format, so the package works with both classic Ember builds and Embroider-optimized builds and no longer depends on@embroider/macrosat runtime. See #19229 for the full description of the migration.Why a new branch
The original PR was ~1900 commits behind
developand conflicting. Rather than resolve the same conflicts twice across its two commits, I squashed them into one and rebased against the final state once.The subtle part: the migration renamed
packages/ember/addon/→src/. Git therefore saw develop's later behavioral changes to those files as edits to deleted files and did not surface them as conflicts. Several develop-side changes had to be ported into the newsrc/files by hand:functionandui.taskspan ops for route hooks and runloop #22669, feat(angular,ember,sveltekit)!: Userouterspan op for frontend routers #23086) — route hooks now emitop: 'function'with acode.function.nameattribute; the runloop usesui.task; the transition span usesrouter.instrumentRoutePerformance.tsstill carried the oldui.ember.route.*ops and had to be updated.url.fullattribute to core fetch instrumentation #22415) —url.path/url.full/url.templateon router spans, reconciled onto the PR's restructuredinstrumentEmberAppInstanceForPerformance.ts.Build/tooling reconciliation
typescript: ~5.8.0devDependency pin.developupgraded to TypeScript 7 (the native compiler, which dropstypescript/lib/tsc), and glint's declaration build needs the classic JS compiler — the same stop-gapdevelopalready applies to ember (see chore(v11): Upgrade to TypeScript 7.0 #19435). Without the pin the declaration build fails withERR_PACKAGE_PATH_NOT_EXPORTED.@sentry/browser/@sentry/corefrom the PR's stale10.53.1to10.67.0and added@sentry/conventions(now imported by the ported instrumentation).import/no-unresolvedoxlint rule (doesn't exist in this repo's oxlint 1.75) and wrappedURL_FULLinfilterCollectedUrl()for thesdk/no-unfiltered-url-attributesrule, which now applies since the code lives undersrc/**.Test fixes
The originally-failing tests came down to three things:
captures correct spans for navigationassertions.traceLifecycle: 'static'in the two new e2e apps (ember-strict-resolver,ember-vite).developmade span-streaming the default and disables it in the ember test apps (test: Disable span streaming in remaining tests #22588); the new apps predated that, so their performance tests hung waiting for transaction events that never arrived under streaming.ember-strict-resolver'ssentry-performance.test.ts, updated from the oldui.ember.*op schema to the newrouter/function/ui.taskops.All four ember e2e apps pass (
ember-classic6/6,ember-embroider6/6,ember-strict-resolver10/10,ember-vite5/5), along with the ember unit tests, lint, and build.Supersedes #19229.