more tweaks to hackathon project - #1935
Open
daniel-graham-amplitude wants to merge 2 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.
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.
Reviewed by Cursor Bugbot for commit 8e1a122. Configure 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.
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
Low Risk
Changes are confined to test-server configurator tooling and the dev Chrome extension; no production SDK or auth paths are modified.
Overview
Extends the configurator hackathon so “Run on URL” is easier to install and behaves more like a real first visit.
Runner extension gains Mock Referrer (shadows
document.referreron the landing page only) and Clean Session (clearsAMP_/amp_cookies and web storage before inject, also once per run viatakePayload()). The background worker is hardened for missing tabs, prerender tab replacement, and clearer errors; the configurator bridge responds when a reload orphans the content script.Distribution & hosted configurator: a Vite plugin (
extension-archive.js) servesconfigurator-extension.zipand a version JSON on dev/build; the UI adds a runner install panel with download steps and stale vs shipped version warnings. The manifest allows the Netlify configurator origin for the bridge.Configurator form: a Blades picker gates Session Replay / Guides sections; Run on URL moves into its own panel (target URL, mock referrer, clean session) with the button disabled until the extension is detected; share-link state includes those run fields.
Reviewed by Cursor Bugbot for commit 067ea30. Bugbot is set up for automated code reviews on this repo. Configure here.