From 8f78bac1d2e699c27ee99f32710562581ddb3857 Mon Sep 17 00:00:00 2001 From: Sidney von Katzendame Date: Sat, 23 May 2026 22:23:37 -0400 Subject: [PATCH 1/2] feat: opt-in tracking-param stripping in normalizeUrl + settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a stripTrackingParams option to @gitmarks/core's normalizeUrl that removes well-known tracking parameters (utm_*, fbclid, gclid, msclkid, mc_*) when the user enables it. Default off — spec.md §'Open questions' deliberately specified this as opt-in to preserve legitimate uses. Wiring: - core/src/url.ts: NormalizeUrlOptions param; case-insensitive match - core/test/url.test.ts: 6 new tests - extension settings.ts: stripTrackingParams: boolean (z default false) - bookmark-factory: pipes the option through to normalizeUrl - save-flow: SaveOptions param the popup passes from settings - popup.ts: reads settings.stripTrackingParams when saving - listeners.ts: ListenerDeps.getStripTrackingParams; applyBatch pipes through to normalizeUrl on both create and url-change update - reconcile.ts: stripTrackingParams param on local-only push - background.ts: provides getStripTrackingParams in ListenerDeps; passes the flag to reconcile() - options.html: checkbox UI for the setting - options.ts: read/write the form field - test/bookmark-factory.test.ts: 2 new tests (strip + preserve) - test/settings.test.ts: legacy-data default test + fixture updates TDD: started with core/test/url.test.ts (RED: 5 failing tests against the new option). Implemented in core. Extended bookmark-factory, then all the wiring. Full suite: core 65/65 (+6), extension 97/97 (+3). Closes #6 --- packages/core/src/url.ts | 33 ++++++++++++- packages/core/test/url.test.ts | 48 +++++++++++++++++++ packages/extension-chrome/src/background.ts | 6 ++- .../src/lib/bookmark-factory.ts | 6 ++- .../extension-chrome/src/lib/listeners.ts | 10 ++-- .../extension-chrome/src/lib/reconcile.ts | 3 +- .../extension-chrome/src/lib/save-flow.ts | 8 ++++ packages/extension-chrome/src/lib/settings.ts | 1 + packages/extension-chrome/src/options.html | 5 ++ packages/extension-chrome/src/options.ts | 3 ++ packages/extension-chrome/src/popup.ts | 1 + .../test-results/.last-run.json | 4 ++ .../test/bookmark-factory.test.ts | 21 ++++++++ .../extension-chrome/test/settings.test.ts | 19 +++++++- 14 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 packages/extension-chrome/test-results/.last-run.json diff --git a/packages/core/src/url.ts b/packages/core/src/url.ts index 7dab27e..1a79e9a 100644 --- a/packages/core/src/url.ts +++ b/packages/core/src/url.ts @@ -1,4 +1,27 @@ -export function normalizeUrl(input: string): string { +// Well-known tracking parameter names. Compared case-insensitively. +// Sources: utm_* (Google Analytics), fbclid (Facebook), gclid (Google Ads), +// msclkid (Microsoft Ads), mc_cid/mc_eid (Mailchimp). +const TRACKING_PARAM_PREFIXES = ["utm_"]; +const TRACKING_PARAM_EXACT = new Set([ + "fbclid", + "gclid", + "msclkid", + "mc_cid", + "mc_eid", +]); + +function isTrackingParam(name: string): boolean { + const lower = name.toLowerCase(); + if (TRACKING_PARAM_EXACT.has(lower)) return true; + return TRACKING_PARAM_PREFIXES.some((p) => lower.startsWith(p)); +} + +export interface NormalizeUrlOptions { + /** Strip well-known tracking params (utm_*, fbclid, gclid, msclkid, mc_*). Default false. */ + stripTrackingParams?: boolean; +} + +export function normalizeUrl(input: string, opts: NormalizeUrlOptions = {}): string { const u = new URL(input); if (u.hash && !u.hash.startsWith("#!")) { @@ -9,5 +32,13 @@ export function normalizeUrl(input: string): string { u.pathname = u.pathname.replace(/\/+$/, ""); } + if (opts.stripTrackingParams) { + const keysToDelete: string[] = []; + for (const name of u.searchParams.keys()) { + if (isTrackingParam(name)) keysToDelete.push(name); + } + for (const name of keysToDelete) u.searchParams.delete(name); + } + return u.toString(); } diff --git a/packages/core/test/url.test.ts b/packages/core/test/url.test.ts index 49df336..ca83ec7 100644 --- a/packages/core/test/url.test.ts +++ b/packages/core/test/url.test.ts @@ -57,4 +57,52 @@ describe("normalizeUrl", () => { it("throws on an invalid URL", () => { expect(() => normalizeUrl("not a url")).toThrow(); }); + + describe("stripTrackingParams option", () => { + it("default (off): preserves utm_* params", () => { + expect(normalizeUrl("https://example.com/?utm_source=foo")).toBe( + "https://example.com/?utm_source=foo", + ); + }); + + it("with stripTrackingParams: removes utm_* params", () => { + expect( + normalizeUrl("https://example.com/?utm_source=foo&utm_medium=bar", { + stripTrackingParams: true, + }), + ).toBe("https://example.com/"); + }); + + it("with stripTrackingParams: removes fbclid, gclid, msclkid, mc_*", () => { + expect( + normalizeUrl("https://example.com/?fbclid=a&gclid=b&msclkid=c&mc_cid=d&mc_eid=e", { + stripTrackingParams: true, + }), + ).toBe("https://example.com/"); + }); + + it("with stripTrackingParams: preserves non-tracking params", () => { + expect( + normalizeUrl("https://example.com/?utm_source=foo&q=real&page=2", { + stripTrackingParams: true, + }), + ).toBe("https://example.com/?q=real&page=2"); + }); + + it("with stripTrackingParams: ignores parameter case (utm_Source treated as tracking)", () => { + expect( + normalizeUrl("https://example.com/?UTM_SOURCE=foo&Utm_Medium=bar", { + stripTrackingParams: true, + }), + ).toBe("https://example.com/"); + }); + + it("stripping leaves an empty query string off the URL", () => { + expect( + normalizeUrl("https://example.com/path?utm_source=x", { + stripTrackingParams: true, + }), + ).toBe("https://example.com/path"); + }); + }); }); diff --git a/packages/extension-chrome/src/background.ts b/packages/extension-chrome/src/background.ts index 4db0621..6db3b93 100644 --- a/packages/extension-chrome/src/background.ts +++ b/packages/extension-chrome/src/background.ts @@ -66,7 +66,7 @@ async function maybeReconcile(): Promise { const idMap = await IdMap.load(); const machineId = await getMachineId(); const nowIso = new Date().toISOString(); - await reconcile(client, idMap, bar, other, machineId, nowIso); + await reconcile(client, idMap, bar, other, machineId, nowIso, settings.stripTrackingParams); }, setStorage: (items) => chrome.storage.local.set(items), removeStorage: (key) => chrome.storage.local.remove(key), @@ -104,6 +104,10 @@ registerListeners({ getIdMap: async () => IdMap.load(), getBarOtherIds, getMachineId, + getStripTrackingParams: async () => { + const s = await loadSettings(); + return s?.stripTrackingParams ?? false; + }, }); chrome.alarms.create(POLL_ALARM_NAME, { periodInMinutes: 5 }); diff --git a/packages/extension-chrome/src/lib/bookmark-factory.ts b/packages/extension-chrome/src/lib/bookmark-factory.ts index 530ac66..e3ad7a6 100644 --- a/packages/extension-chrome/src/lib/bookmark-factory.ts +++ b/packages/extension-chrome/src/lib/bookmark-factory.ts @@ -6,12 +6,16 @@ export interface BuildBookmarkInput { title: string; machineId: string; nowIso: string; + /** Strip tracking params (utm_, fbclid, gclid, etc.) at save time. Default false. */ + stripTrackingParams?: boolean; } export function buildBookmark(input: BuildBookmarkInput): Bookmark { return { id: newUlid(), - url: normalizeUrl(input.url), + url: normalizeUrl(input.url, { + stripTrackingParams: input.stripTrackingParams ?? false, + }), title: input.title, folder: "", tags: [], diff --git a/packages/extension-chrome/src/lib/listeners.ts b/packages/extension-chrome/src/lib/listeners.ts index ea3e2c2..7ef2992 100644 --- a/packages/extension-chrome/src/lib/listeners.ts +++ b/packages/extension-chrome/src/lib/listeners.ts @@ -29,6 +29,8 @@ export interface ListenerDeps { getIdMap: () => Promise; getBarOtherIds: () => Promise<{ bar: string; other: string }>; getMachineId: () => Promise; + /** Defaults to false if not provided (preserves current behavior in tests). */ + getStripTrackingParams?: () => Promise; } let pending: Pending[] = []; @@ -145,6 +147,7 @@ export async function flushPending(): Promise { const idMap = await deps.getIdMap(); const machineId = await deps.getMachineId(); + const stripTrackingParams = (await deps.getStripTrackingParams?.()) ?? false; const nowIso = new Date().toISOString(); // A node that has a create event in this same batch should accept @@ -208,7 +211,7 @@ export async function flushPending(): Promise { (current) => { toAdd.length = 0; toRemove.length = 0; - return applyBatch(current, surviving, idMap, createUlids, machineId, nowIso, toAdd, toRemove); + return applyBatch(current, surviving, idMap, createUlids, machineId, nowIso, stripTrackingParams, toAdd, toRemove); }, `sync ${surviving.length} change(s) from chrome@${machineId}`, machineId, @@ -235,6 +238,7 @@ function applyBatch( createUlids: Map, machineId: string, nowIso: string, + stripTrackingParams: boolean, toAdd: Array<{ ulid: string; nodeId: string }>, toRemove: string[], ): BookmarksFile { @@ -247,7 +251,7 @@ function applyBatch( if (id == null) continue; const bm: Bookmark = { id, - url: normalizeUrl(event.url), + url: normalizeUrl(event.url, { stripTrackingParams }), title: event.title, folder: "", tags: [], @@ -270,7 +274,7 @@ function applyBatch( if (existing == null) continue; const patch: Partial> = {}; if ("url" in event && event.url !== undefined) { - const normalized = normalizeUrl(event.url); + const normalized = normalizeUrl(event.url, { stripTrackingParams }); if (normalized !== existing.url) patch.url = normalized; } if ("title" in event && event.title !== undefined) { diff --git a/packages/extension-chrome/src/lib/reconcile.ts b/packages/extension-chrome/src/lib/reconcile.ts index 47b2101..e1d4385 100644 --- a/packages/extension-chrome/src/lib/reconcile.ts +++ b/packages/extension-chrome/src/lib/reconcile.ts @@ -26,6 +26,7 @@ export async function reconcile( otherBookmarksId: string, machineId: string, nowIso: string, + stripTrackingParams = false, ): Promise { let remote: BookmarksFile; try { @@ -70,7 +71,7 @@ export async function reconcile( const id = newUlid(); const bm: Bookmark = { id, - url: normalizeUrl(local.url), + url: normalizeUrl(local.url, { stripTrackingParams }), title: local.title, folder: "", tags: [], diff --git a/packages/extension-chrome/src/lib/save-flow.ts b/packages/extension-chrome/src/lib/save-flow.ts index 28d1a99..730b351 100644 --- a/packages/extension-chrome/src/lib/save-flow.ts +++ b/packages/extension-chrome/src/lib/save-flow.ts @@ -25,17 +25,25 @@ export type SaveResult = message: string; }; +export interface SaveOptions { + stripTrackingParams?: boolean; +} + export async function saveBookmark( client: GitHubClient, page: PageInfo, machineId: string, nowIso: string, + opts: SaveOptions = {}, ): Promise { const bookmark = buildBookmark({ url: page.url, title: page.title, machineId, nowIso, + ...(opts.stripTrackingParams !== undefined + ? { stripTrackingParams: opts.stripTrackingParams } + : {}), }); const commitMsg = `add bookmark from chrome@${machineId}`; diff --git a/packages/extension-chrome/src/lib/settings.ts b/packages/extension-chrome/src/lib/settings.ts index 59789d4..7dd7854 100644 --- a/packages/extension-chrome/src/lib/settings.ts +++ b/packages/extension-chrome/src/lib/settings.ts @@ -7,6 +7,7 @@ export const settingsSchema = z.object({ owner: z.string().regex(/^[A-Za-z0-9_.-]+$/, "owner must be a single GitHub login"), repo: z.string().regex(/^[A-Za-z0-9_.-]+$/, "repo must be a single GitHub repo name"), branch: z.string().min(1), + stripTrackingParams: z.boolean().default(false), }); export type Settings = z.infer; diff --git a/packages/extension-chrome/src/options.html b/packages/extension-chrome/src/options.html index e219e0b..16309e7 100644 --- a/packages/extension-chrome/src/options.html +++ b/packages/extension-chrome/src/options.html @@ -47,6 +47,11 @@

gitmarks settings

+ +
diff --git a/packages/extension-chrome/src/options.ts b/packages/extension-chrome/src/options.ts index 9485eed..3b2e656 100644 --- a/packages/extension-chrome/src/options.ts +++ b/packages/extension-chrome/src/options.ts @@ -16,6 +16,7 @@ const tokenInput = $("token"); const ownerInput = $("owner"); const repoInput = $("repo"); const branchInput = $("branch"); +const stripTrackingParamsInput = $("stripTrackingParams"); const validateBtn = $("validate"); const saveBtn = $("save"); const status = $("status"); @@ -26,6 +27,7 @@ function readForm(): Settings { owner: ownerInput.value.trim(), repo: repoInput.value.trim(), branch: branchInput.value.trim() || "main", + stripTrackingParams: stripTrackingParamsInput.checked, }; } @@ -55,6 +57,7 @@ async function loadIntoForm(): Promise { ownerInput.value = s.owner; repoInput.value = s.repo; branchInput.value = s.branch; + stripTrackingParamsInput.checked = s.stripTrackingParams; } validateBtn.addEventListener("click", async () => { diff --git a/packages/extension-chrome/src/popup.ts b/packages/extension-chrome/src/popup.ts index 26594d2..b3f6c84 100644 --- a/packages/extension-chrome/src/popup.ts +++ b/packages/extension-chrome/src/popup.ts @@ -94,6 +94,7 @@ async function render(): Promise { { url: tab.url!, title: tab.title ?? tab.url! }, machineId, new Date().toISOString(), + { stripTrackingParams: settings.stripTrackingParams }, ); } catch (err) { result = { diff --git a/packages/extension-chrome/test-results/.last-run.json b/packages/extension-chrome/test-results/.last-run.json new file mode 100644 index 0000000..cbcc1fb --- /dev/null +++ b/packages/extension-chrome/test-results/.last-run.json @@ -0,0 +1,4 @@ +{ + "status": "passed", + "failedTests": [] +} \ No newline at end of file diff --git a/packages/extension-chrome/test/bookmark-factory.test.ts b/packages/extension-chrome/test/bookmark-factory.test.ts index 1f95bca..fa8498b 100644 --- a/packages/extension-chrome/test/bookmark-factory.test.ts +++ b/packages/extension-chrome/test/bookmark-factory.test.ts @@ -55,6 +55,27 @@ describe("buildBookmark", () => { expect(bm.updated_at).toBe("2026-05-23T14:32:11Z"); }); + it("strips tracking params when stripTrackingParams is true (issue #6)", () => { + const bm = buildBookmark({ + url: "https://example.com/?utm_source=feed&q=real", + title: "Article", + machineId: "ABCDE12F", + nowIso: "2026-05-23T14:32:11Z", + stripTrackingParams: true, + }); + expect(bm.url).toBe("https://example.com/?q=real"); + }); + + it("preserves tracking params when stripTrackingParams is false (default)", () => { + const bm = buildBookmark({ + url: "https://example.com/?utm_source=feed", + title: "Article", + machineId: "ABCDE12F", + nowIso: "2026-05-23T14:32:11Z", + }); + expect(bm.url).toBe("https://example.com/?utm_source=feed"); + }); + it("generates a fresh ULID each call", () => { const a = buildBookmark({ url: "https://example.com/", diff --git a/packages/extension-chrome/test/settings.test.ts b/packages/extension-chrome/test/settings.test.ts index acd8ab2..f6e9def 100644 --- a/packages/extension-chrome/test/settings.test.ts +++ b/packages/extension-chrome/test/settings.test.ts @@ -12,11 +12,26 @@ describe("settings", () => { owner: "alice", repo: "bookmarks", branch: "main", + stripTrackingParams: false, }; await saveSettings(s); expect(await loadSettings()).toEqual(s); }); + it("defaults stripTrackingParams to false when omitted in stored data", async () => { + // Legacy stored settings without the field should still parse via Zod's default. + await chrome.storage.local.set({ + "gitmarks:settings": { + token: "t", + owner: "alice", + repo: "bookmarks", + branch: "main", + }, + }); + const loaded = await loadSettings(); + expect(loaded?.stripTrackingParams).toBe(false); + }); + it("throws SettingsCorruptError when the stored value is malformed", async () => { await chrome.storage.local.set({ "gitmarks:settings": { not: "valid" } }); await expect(loadSettings()).rejects.toThrow(/invalid/); @@ -28,6 +43,7 @@ describe("settings", () => { owner: "o", repo: "r", branch: "main", + stripTrackingParams: false, }); await clearSettings(); expect(await loadSettings()).toBeNull(); @@ -35,7 +51,7 @@ describe("settings", () => { it("rejects an empty token at save time", async () => { await expect( - saveSettings({ token: "", owner: "o", repo: "r", branch: "main" }), + saveSettings({ token: "", owner: "o", repo: "r", branch: "main", stripTrackingParams: false }), ).rejects.toThrow(); }); @@ -46,6 +62,7 @@ describe("settings", () => { owner: "a/b", repo: "r", branch: "main", + stripTrackingParams: false, }), ).rejects.toThrow(); }); From 03d9c0a987dfda843dbb1c21bce4f6d487f0a92d Mon Sep 17 00:00:00 2001 From: Sidney von Katzendame Date: Sat, 23 May 2026 22:24:02 -0400 Subject: [PATCH 2/2] chore: gitignore playwright test-results + remove accidentally-tracked artifact --- .gitignore | 2 ++ packages/extension-chrome/test-results/.last-run.json | 4 ---- 2 files changed, 2 insertions(+), 4 deletions(-) delete mode 100644 packages/extension-chrome/test-results/.last-run.json diff --git a/.gitignore b/.gitignore index a2e5ab6..28e34ae 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,5 @@ dist/ .idea/ coverage/ .turbo/ +test-results/ +playwright-report/ diff --git a/packages/extension-chrome/test-results/.last-run.json b/packages/extension-chrome/test-results/.last-run.json deleted file mode 100644 index cbcc1fb..0000000 --- a/packages/extension-chrome/test-results/.last-run.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "status": "passed", - "failedTests": [] -} \ No newline at end of file