Uh oh!
There was an error while loading. Please reload this page.
fix(carbon-ads): keep the script id resolvable after unmount - #872
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
harlan-zw
commented
Aug 20, 2026
Checked in a real browser against live carbon.js, since CI has no Carbon coverage. Mount, unmount, remount, then three Without this change, after unmount: With it, after unmount: Remount and the three key cycles kept exactly one script element and one rendered ad, with no page errors. |
commit: |
📦 Package Size✅ No notable size changes 📚 22 runtime dependencies (no change) All tracked output (27)
Runtime dependencies (22)
Baseline: main_@_a423e821___2026-08-20 · gzip is the comparison metric · changes below 16 B gzip are ignored |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe Carbon Ads component now initializes its element reference as Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk:⚪ Minimal · up to This localized lifecycle fix keeps the script reference resolvable after unmount and adds coverage for the behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
📚 Description
Unmounting
ScriptCarbonAdscrashes the page. carbon.js resolves#_carbonads_jsfrom its own pending ad callback and reads.srcoff it with no null check, so removing that element throws inside third-party code where nothing can catch it. Sites that key the ad on the route to refresh it per page hit this on every navigation.The root ref was also seeded from
document.getElementById('_carbonads_js'), which is the script's id rather than a container. Every other registry component starts that ref atnull.Open question: two instances mounted at once still collide on the hardcoded id. Carbon's own code assumes one per page, so I left it alone.