Improved Sentry diagnostics for Billing app load failures#27937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR enhances the billing service's iframe load tracking and failure diagnostics. It introduces state fields to track when the iframe src is set and whether the load event fires, adds a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
53dd752 to
024da0e
Compare
024da0e to
c2135f6
Compare
c2135f6 to
a6b49d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/admin/app/services/billing.js`:
- Around line 111-116: The boolean _loadListenerAttached is global and can be
stale when the `#billing-frame` iframe is re-created, causing the new iframe to
miss its load handler and billingAppIframeLoadFired to be wrong; instead, check
and mark the listener on the iframe instance itself (e.g. set a property like
iframe.__loadListenerAttached or use iframe.dataset.loadListenerAttached) before
calling iframe.addEventListener('load', ...) and set that instance flag inside
the handler so each new iframe gets its own listener and
billingAppIframeLoadFired is updated reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc1ed012-2983-42b8-8481-e527f9dd5ebc
📒 Files selected for processing (3)
ghost/admin/app/components/gh-billing-iframe.jsghost/admin/app/services/billing.jsghost/admin/tests/unit/services/billing-test.js
ref [INC-284](https://app.incident.io/ghost/incidents/284) Sentry ADMIN-1BZQ (439 events / 294 users) reports `Billing app failed to become ready` with very little context, making it hard to tell whether the iframe is being blocked, the network is offline, the BMA is hanging during boot, or the load monitor is firing while the iframe is still hidden. The Sentry capture in `BillingService.reportBillingAppLoadFailure` now records `document.visibilityState`, the iframe's `offsetParent` visibility, computed `display`/`visibility`, bounding rect, a `__bmaBoot` probe (`accessible` / `has_markReady` / `threw`), whether the iframe's `load` event fired, `ms_since_src_set`, `navigator.onLine`, `navigator.connection.effectiveType`, and `billingWindowOpen`. The event is now reported at `level: 'warning'` (the user-facing error UI is rendered separately by `gh-billing-modal`, so a hard error is the wrong severity) with a stable `fingerprint` keyed on visibility state and attempt count so the enriched buckets stay distinct from the legacy noise. `setBillingIframeSrc` is extracted so both the initial render and the retry path record `billingAppIframeSrcSetAt` and attach the `load` listener — no behavioral change to the monitor itself, which keeps its existing 10s timeout + 1 retry. The hypothesis that the parent-side timer needed a visibility gate (mirroring TryGhost/billing#1209) doesn't hold up: parent `setTimeout` is only throttled when the whole tab is backgrounded, and the Sentry tag distribution shows the failures are not concentrated on mobile, where hidden-iframe throttling would be most aggressive. Better to ship diagnostics first, see what the data actually says, then decide on a behavioral fix.
a6b49d5 to
0724fdf
Compare
ref [INC-284](https://app.incident.io/ghost/incidents/284) Track the iframe instance the `load` listener was attached to instead of a global boolean. If `#billing-frame` is ever re-created (Ember re-render or HMR), the boolean would say "already attached" while the new iframe element has no listener, leaving `billingAppIframeLoadFired` stuck and the Sentry diagnostic misleading. Comparing against the iframe reference re-attaches the listener whenever the element changes. Addresses CodeRabbit review feedback on #27937.
ref INC-284
ref Sentry ADMIN-1BZQ — 439 events / 294 users
Summary
Billing app failed to become ready(ADMIN-1BZQ) currently lands in Sentry with very little context — we can't tell whether the iframe is being blocked, the network is offline, the BMA is hanging during boot, the iframeloadevent never fired, or the load monitor is firing while the iframe is still hidden behind the closed modal.This PR enriches the existing Sentry capture in
BillingService.reportBillingAppLoadFailurewith the data needed to actually diagnose ADMIN-1BZQ. Added fields undercontexts.ghost.billing_monitor:document_visibility_stateiframe_offset_parent_visible,iframe_computed_display,iframe_computed_visibility,iframe_bounding_rectbma_boot_probe—{accessible, has_markReady, threw}againstiframe.contentWindow.__bmaBootiframe_load_fired— whether the iframe'sloadevent ever firedms_since_src_set— how long the parent has been waiting sinceiframe.srcwas assignednavigator_online,connection_effective_typebilling_window_openThe event is also now reported at
level: 'warning'(the user-facing error UI is rendered separately bygh-billing-modal, so a hard error is the wrong severity) with a stablefingerprint: ['billing-app-load-failure', visibilityState, attempts]so enriched events don't merge into the legacy bucket.setBillingIframeSrcis extracted fromgh-billing-iframe.js'ssetup()so both the initial render and the retry path consistently recordbillingAppIframeSrcSetAtand attach theloadlistener. No behavioral change to the monitor itself — same 10s timeout, same one retry.Ship diagnostics, observe what the enriched events actually say, then make an informed behavioral change in a follow-up.
Test plan
pnpm exec eslintclean on changed filesbilling_monitorfields, thewarninglevel, and thefingerprint;bma_boot_probeis asserted by key-shape (value depends on iframe presence)getBillingIframetonullfor deterministic diagnostics (the previous CI failure was caused by a sibling test leaving an iframe in the DOM withid="billing-frame")level: warning