more tweaks to hackathon project - #1935
Open
daniel-graham-amplitude wants to merge 3 commits into
Open
Conversation
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Leftover unused install scaffolding
- Removed the unused checkout-install constants, CodeBlock import, commands style, and the prism-bash import that only existed for those setup commands.
- ✅ Fixed: Tab swap skips SDK injection
- onReplaced now injects into an already-committed replacement tab, and runOnUrl treats a prerender swap as success instead of a missing-tab error.
Or push these changes by commenting:
@cursor push 2f3cd503df
Preview (2f3cd503df)
diff --git a/test-server/configurator-extension/background.js b/test-server/configurator-extension/background.js
--- a/test-server/configurator-extension/background.js
+++ b/test-server/configurator-extension/background.js
@@ -19,6 +19,10 @@
const TABS_KEY = 'instrumentedTabs';
const LAST_PAYLOAD_KEY = 'lastPayload';
+// Filled synchronously at the start of onReplaced so runOnUrl can tell a prerender swap from a close
+// after tabs.update fails on the old id. Dropped on the next turn, once that catch has had a look.
+const replacedTabs = new Map();
+
// What the configurator sends when no API key has been typed in — PLACEHOLDER_API_KEY in its snippet.js.
const PLACEHOLDER_API_KEY = 'YOUR_API_KEY';
@@ -110,44 +114,51 @@
};
}
+async function injectInto(tabId, payload, url) {
+ if (!url?.startsWith('http')) {
+ return;
+ }
+ // Read before injecting: by the time a navigation commits the response headers have arrived, which is
+ // where the policy the page was sent is still visible.
+ const csp = cspReport(tabId, payload);
+ if (csp) {
+ await ignoreMissingTab(chrome.action.setTitle({ tabId, title: csp.summary }));
+ }
+ const target = { tabId };
+ const inject = (options) =>
+ chrome.scripting.executeScript({ target, world: 'MAIN', injectImmediately: true, ...options });
+ try {
+ await inject({ func: handOver, args: [payload, csp] });
+ await inject({ files: [SDK_BUNDLE] });
+ if (payload.sessionReplay) {
+ // Its own call: a plugin bundle that won't load shouldn't stop analytics from running, and
+ // inject.js reports the gap when the global it expects isn't there.
+ try {
+ await inject({ files: [SESSION_REPLAY_BUNDLE] });
+ } catch (error) {
+ console.warn('[amplitude-configurator] session replay bundle failed to load', error);
+ }
+ }
+ await inject({ files: ['inject.js'] });
+ } catch (error) {
+ if (isMissingTab(error)) {
+ throw error;
+ }
+ console.error('[amplitude-configurator] injection failed', error);
+ await ignoreMissingTab(chrome.action.setBadgeText({ tabId, text: 'err' }));
+ }
+}
+
chrome.webNavigation.onCommitted.addListener(
guard('injection', async ({ tabId, frameId, url }) => {
- if (frameId !== 0 || !url.startsWith('http')) {
+ if (frameId !== 0) {
return;
}
const payload = (await instrumentedTabs())[tabId];
if (!payload) {
return;
}
- // Read before injecting: by the time a navigation commits the response headers have arrived, which is
- // where the policy the page was sent is still visible.
- const csp = cspReport(tabId, payload);
- if (csp) {
- await ignoreMissingTab(chrome.action.setTitle({ tabId, title: csp.summary }));
- }
- const target = { tabId };
- const inject = (options) =>
- chrome.scripting.executeScript({ target, world: 'MAIN', injectImmediately: true, ...options });
- try {
- await inject({ func: handOver, args: [payload, csp] });
- await inject({ files: [SDK_BUNDLE] });
- if (payload.sessionReplay) {
- // Its own call: a plugin bundle that won't load shouldn't stop analytics from running, and
- // inject.js reports the gap when the global it expects isn't there.
- try {
- await inject({ files: [SESSION_REPLAY_BUNDLE] });
- } catch (error) {
- console.warn('[amplitude-configurator] session replay bundle failed to load', error);
- }
- }
- await inject({ files: ['inject.js'] });
- } catch (error) {
- if (isMissingTab(error)) {
- throw error;
- }
- console.error('[amplitude-configurator] injection failed', error);
- await ignoreMissingTab(chrome.action.setBadgeText({ tabId, text: 'err' }));
- }
+ await injectInto(tabId, payload, url);
}),
);
@@ -178,8 +189,9 @@
}
// The tab opens blank so it can be marked for instrumentation before it commits anything; navigating
// afterwards is what makes the ordering reliable. It also means there is a moment where the run depends
- // on a tab nobody is looking at yet, and anything that closes it — a click, a tab-tidying extension,
- // Chrome swapping in a prerender — leaves the steps below with nothing to work on.
+ // on a tab nobody is looking at yet, and anything that closes it — a click, a tab-tidying extension —
+ // leaves the steps below with nothing to work on. A prerender swap is different: onReplaced moves the
+ // mark, and the catch below treats that as the run continuing rather than as a failure.
let tab;
try {
tab = await chrome.tabs.create({ url: 'about:blank', active: true });
@@ -190,6 +202,11 @@
if (!isMissingTab(error)) {
throw error;
}
+ // Chrome can swap the blank tab for a prerender of the destination; onReplaced records that
+ // synchronously, and has already moved the mark to the surviving id.
+ if (tab && replacedTabs.has(tab.id)) {
+ return { message: describe(payload) };
+ }
if (tab) {
// The mark and the CSP rule are both keyed by tab id, and Chrome reuses ids, so leaving them behind
// would take the policy off whichever tab inherits this one's.
@@ -234,11 +251,21 @@
// mark and the CSP rule across keeps the run alive, and keeps a rule from outliving the tab it was for.
chrome.tabs.onReplaced.addListener(
guard('tab replacement', async (addedTabId, removedTabId) => {
+ replacedTabs.set(removedTabId, addedTabId);
+ setTimeout(() => replacedTabs.delete(removedTabId), 0);
const payload = (await instrumentedTabs())[removedTabId];
if (!payload) {
return;
}
await forget(removedTabId);
await instrument(addedTabId, payload);
+ // The prerendered document committed under this id before it was marked, so onCommitted will not
+ // run again for this load. Inject into whatever is already there; a document that hasn't committed
+ // yet is left for the forthcoming onCommitted.
+ const frames = await chrome.webNavigation.getAllFrames({ tabId: addedTabId });
+ const url = frames?.find((frame) => frame.frameId === 0)?.url;
+ if (url) {
+ await injectInto(addedTabId, payload, url);
+ }
}),
);
diff --git a/test-server/configurator/components.jsx b/test-server/configurator/components.jsx
--- a/test-server/configurator/components.jsx
+++ b/test-server/configurator/components.jsx
@@ -1,8 +1,5 @@
import React from 'react';
-// Prism's default build already registers the javascript and markup grammars, so only the shell one the
-// extension's setup commands are shown in has to be pulled in.
import Prism from 'prismjs';
-import 'prismjs/components/prism-bash';
import './syntax-theme.css';
const styles = {
diff --git a/test-server/configurator/runner-extension-panel.jsx b/test-server/configurator/runner-extension-panel.jsx
--- a/test-server/configurator/runner-extension-panel.jsx
+++ b/test-server/configurator/runner-extension-panel.jsx
@@ -2,12 +2,8 @@
// install link to point at: the steps are the download this server builds, and Load unpacked. They mirror
// test-server/configurator-extension/README.md, which is the fuller account.
import React from 'react';
-import { CodeBlock, Panel } from './components.jsx';
+import { Panel } from './components.jsx';
-const EXTENSION_DIRECTORY = 'test-server/configurator-extension';
-
-const REPOSITORY_URL = `https://github.com/amplitude/Amplitude-TypeScript/tree/main/${EXTENSION_DIRECTORY}`;
-
// Built by test-server/extension-archive.js, which owns this path, out of the extension directory as it
// stands in whatever checkout is serving this page.
const ARCHIVE_URL = '/configurator-extension.zip';
@@ -16,18 +12,11 @@
// read the same either way.
const UNPACKED_FOLDER = 'configurator-extension';
-// The bundles the extension injects aren't checked in. The archive carries them already; a checkout has
-// to build them before Chrome will accept the folder.
-const SETUP_COMMANDS = `pnpm --dir packages/analytics-browser build
-pnpm --dir packages/plugin-session-replay-browser build
-node ${EXTENSION_DIRECTORY}/sync-vendor.mjs`;
-
const styles = {
wrapper: { maxWidth: 760, margin: '0 0 20px' },
note: { color: '#888', fontSize: 12, margin: '0 0 10px' },
steps: { margin: '0 0 10px', paddingLeft: 20, fontSize: 13, color: '#444', lineHeight: 1.6 },
step: { marginBottom: 6 },
- commands: { margin: '8px 0 4px' },
};
export function RunnerExtensionPanel({ version }) {You can send follow-ups to the cloud agent here.
| steps: { margin: '0 0 10px', paddingLeft: 20, fontSize: 13, color: '#444', lineHeight: 1.6 }, | ||
| step: { marginBottom: 6 }, | ||
| commands: { margin: '8px 0 4px' }, | ||
| }; |
There was a problem hiding this comment.
Leftover unused install scaffolding
Low Severity
REPOSITORY_URL, SETUP_COMMANDS, EXTENSION_DIRECTORY, the unused CodeBlock import, and styles.commands look like leftovers from a checkout-based install path that never made it into the panel. The new prism-bash import in components.jsx is only useful for those unused setup commands.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8e1a122. Configure here.
| await forget(removedTabId); | ||
| await instrument(addedTabId, payload); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Tab swap skips SDK injection
Medium Severity
The new onReplaced handler moves the instrumentation mark and CSP rule to the replacement tab, but injection only runs from onCommitted. After a prerender swap that commit has already happened on the new id, so the badge can read on while the page never gets the SDK.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8e1a122. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Tab storage update race
- Serialized instrument, forget, and takePayload so overlapping session-storage read-modify-writes can no longer drop another tab's mark.
Or push these changes by commenting:
@cursor push 255428c300
Preview (255428c300)
diff --git a/test-server/configurator-extension/background.js b/test-server/configurator-extension/background.js
--- a/test-server/configurator-extension/background.js
+++ b/test-server/configurator-extension/background.js
@@ -55,6 +55,21 @@
return tabs;
}
+// chrome.storage.session has no atomic update, so a get that yields and a later set of the whole map
+// would write a snapshot that no longer has every tab. instrument, forget and takePayload all do
+// that — takePayload on nearly every first commit, because clearSession defaults to true — so they
+// share a queue, and each re-reads after the previous write has landed.
+let tabsQueue = Promise.resolve();
+
+function withTabs(work) {
+ const run = tabsQueue.then(work);
+ tabsQueue = run.then(
+ () => {},
+ () => {},
+ );
+ return run;
+}
+
// A tab is not a thing that stays put: it can be closed, and Chrome can swap it for a prerendered one
// mid-navigation. Every call below that names a tab id can therefore find nothing there, and tabs, action
// and scripting all say so the same way — "No tab with id: 1234.", a message that says nothing about what
@@ -92,16 +107,20 @@
// Both transitions carry the CSP rule with them, so no path can mark a tab and forget to clear the way for
// what the SDK is about to do — or leave a tab unprotected after instrumentation stops.
async function instrument(tabId, payload) {
- const tabs = await instrumentedTabs();
- await chrome.storage.session.set({ [TABS_KEY]: { ...tabs, [tabId]: payload } });
+ await withTabs(async () => {
+ const tabs = await instrumentedTabs();
+ await chrome.storage.session.set({ [TABS_KEY]: { ...tabs, [tabId]: payload } });
+ });
await relaxCsp(tabId);
await ignoreMissingTab(chrome.action.setBadgeText({ tabId, text: 'on' }));
}
async function forget(tabId) {
- const tabs = await instrumentedTabs();
- delete tabs[tabId];
- await chrome.storage.session.set({ [TABS_KEY]: tabs });
+ await withTabs(async () => {
+ const tabs = await instrumentedTabs();
+ delete tabs[tabId];
+ await chrome.storage.session.set({ [TABS_KEY]: tabs });
+ });
await restoreCsp(tabId);
}
@@ -118,16 +137,18 @@
// tab and the navigation it opened, which would otherwise make "first commit" mean "first since the worker
// last woke up".
async function takePayload(tabId) {
- const tabs = await instrumentedTabs();
- const payload = tabs[tabId];
- if (!payload) {
- return undefined;
- }
- const { clearSession, mockReferrer, ...rest } = payload;
- if (clearSession || mockReferrer) {
- await chrome.storage.session.set({ [TABS_KEY]: { ...tabs, [tabId]: rest } });
- }
- return payload;
+ return withTabs(async () => {
+ const tabs = await instrumentedTabs();
+ const payload = tabs[tabId];
+ if (!payload) {
+ return undefined;
+ }
+ const { clearSession, mockReferrer, ...rest } = payload;
+ if (clearSession || mockReferrer) {
+ await chrome.storage.session.set({ [TABS_KEY]: { ...tabs, [tabId]: rest } });
+ }
+ return payload;
+ });
}
// Runs in the page before the SDK bundle: saves what the page had under window.amplitude, since theYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 9b728c0. Configure here.
| await chrome.storage.session.set({ [TABS_KEY]: { ...tabs, [tabId]: rest } }); | ||
| } | ||
| return payload; | ||
| } |
There was a problem hiding this comment.
Tab storage update race
Medium Severity
takePayload read-modify-writes the whole instrumentedTabs map from a snapshot. A concurrent instrument or forget for another tab can be overwritten, so a newly marked run can lose its mark and never get injected. Because clearSession defaults to true, that write happens on nearly every first commit.
Reviewed by Cursor Bugbot for commit 9b728c0. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Checklist
Note
Medium Risk
The runner still injects the SDK on arbitrary origins and can clear Amplitude cookies/storage in the user's browser; changes are confined to test-server tooling, not shipped SDK packages.
Overview
Extends the configurator hackathon runner with a fuller Run on URL flow, easier extension distribution, and harder-to-break Chrome extension behavior.
The configurator gains a Run on URL panel (target URL, optional mock referrer, default-on clean session) whose values ride in share links but not generated snippets. Run on URL stays disabled until the runner is detected; a new install panel downloads
configurator-extension.zipfrom the dev server or static build and compares installed vs shipped manifest versions. Product toggles move to a Blades row that gates Session Replay and Guides sections.The runner extension implements mock referrer (shadow
document.referreron the landing page only) and Amplitude-only storage wipe before SDK init, withtakePayload()consuming those flags on the first navigation commit. Service worker handling adds missing-tab tolerance,tabs.onReplacedmigration, and guarded async listeners; the configurator bridge fails fast when the page is stale after an extension reload. Vite addsextension-archive.jsto zip the extension (vendor bundles required) and serve version JSON;sync-vendor.mjsvendors source maps; manifest adds a hosted Netlify configurator origin andweb_accessible_resourcesfor maps.Reviewed by Cursor Bugbot for commit 9b728c0. Bugbot is set up for automated code reviews on this repo. Configure here.