diff --git a/packages/extension-chrome/README.md b/packages/extension-chrome/README.md index 6bb75e5..fae012f 100644 --- a/packages/extension-chrome/README.md +++ b/packages/extension-chrome/README.md @@ -41,9 +41,11 @@ at build time (see "Icons" below). 2. Click the toolbar icon → "Set up gitmarks" → enter PAT, owner, repo, branch. 3. Optionally enable **"Strip tracking parameters"** to remove `utm_*`, - `fbclid`, `gclid`, `msclkid`, and `mc_*` from saved URLs. Default off - per the spec's open-question rationale (some sites use `utm_*` for - non-tracking purposes). + `fbclid`, `gclid`, `msclkid`, and `mc_*` from saved URLs. Links shared + from social posts or newsletters carry these; stripping them means the + same article saved from two sources collapses to one bookmark, not two. + Default off per the spec's open-question rationale (some sites use + `utm_*` for non-tracking purposes). 4. Click **Validate**. You should see either "✓ valid PAT, repo exists, bookmarks.json found" or "✓ valid PAT, repo exists (bookmarks.json not yet created — will be on diff --git a/packages/extension-shared/src/lib/save-result-view.ts b/packages/extension-shared/src/lib/save-result-view.ts new file mode 100644 index 0000000..b864db2 --- /dev/null +++ b/packages/extension-shared/src/lib/save-result-view.ts @@ -0,0 +1,36 @@ +import type { SaveResult } from "./save-flow.js"; + +/** + * Apply the outcome of a popup save to the button + status line. + * + * On success: show "✓ saved", relabel the button to "Saved ✓" and mark it + * `done` (the CSS for `.done` cancels the disabled progress-cursor), then + * auto-dismiss the popup after `closeDelayMs` so it gets out of the way. On + * failure: surface the message and re-enable the button to retry. + * + * Extracted from popup.ts so these transitions are unit-testable without the + * popup entry's top-level DOM bootstrap (issue #43). The success branch + * previously left the button stuck on "saving…" with a progress cursor. + */ +export function applySaveResult( + saveBtn: HTMLButtonElement, + status: HTMLElement, + result: SaveResult, + options: { closePopup?: () => void; closeDelayMs?: number } = {}, +): void { + const { closePopup = () => window.close(), closeDelayMs = 1200 } = options; + if (result.ok) { + status.className = "ok"; + status.textContent = "✓ saved"; + saveBtn.textContent = "Saved ✓"; + saveBtn.classList.add("done"); + // Delay leaves screen-reader users time to hear the status update; a no-op + // when opened as a plain page (e2e) where window.close() does nothing. + setTimeout(closePopup, closeDelayMs); + } else { + status.className = "err"; + status.textContent = result.message; + saveBtn.disabled = false; + saveBtn.textContent = "Try again"; + } +} diff --git a/packages/extension-shared/src/options.html b/packages/extension-shared/src/options.html index 16309e7..670347a 100644 --- a/packages/extension-shared/src/options.html +++ b/packages/extension-shared/src/options.html @@ -4,24 +4,51 @@ gitmarks — settings @@ -51,6 +78,11 @@

gitmarks settings

Strip tracking parameters (utm_*, fbclid, gclid, msclkid, mc_*) at save time +

+ Links shared from social posts or newsletters often carry tracking + parameters that identify who shared them. Stripping these means the same + article saved from two sources becomes one bookmark, not two. +

diff --git a/packages/extension-shared/src/popup.html b/packages/extension-shared/src/popup.html index 1f68f33..f84682a 100644 --- a/packages/extension-shared/src/popup.html +++ b/packages/extension-shared/src/popup.html @@ -35,6 +35,8 @@ } button:hover:not(:disabled) { background: #22d3ee; color: #0a0a0f; } button:disabled { opacity: 0.5; cursor: progress; } + /* Success state: stays disabled (no re-save) but reads as done, not busy. */ + button.done { opacity: 1; cursor: default; background: transparent; color: #22d3ee; } #status { margin-top: 0.75rem; font-size: 0.85rem; min-height: 1.2em; } #status.ok { color: #22d3ee; } #status.err { color: #e879f9; } diff --git a/packages/extension-shared/src/popup.ts b/packages/extension-shared/src/popup.ts index 193ede8..686dd50 100644 --- a/packages/extension-shared/src/popup.ts +++ b/packages/extension-shared/src/popup.ts @@ -3,6 +3,7 @@ import { GitHubClient } from "@gitmarks/core"; import { loadSettings, SettingsCorruptError } from "./lib/settings.js"; import { getMachineId } from "./lib/machine-id.js"; import { saveBookmark, type SaveResult } from "./lib/save-flow.js"; +import { applySaveResult } from "./lib/save-result-view.js"; import type { LastErrorRecord } from "./lib/background-core.js"; const root = document.getElementById("root"); @@ -104,18 +105,7 @@ async function render(): Promise { message: err instanceof Error ? err.message : String(err), }; } - if (result.ok) { - status.className = "ok"; - status.textContent = "✓ saved"; - // Auto-dismiss the popup shortly after a successful save so the user - // doesn't have to click away. No-op when opened as a plain page (e2e). - setTimeout(() => window.close(), 1200); - } else { - status.className = "err"; - status.textContent = result.message; - saveBtn.disabled = false; - saveBtn.textContent = "Try again"; - } + applySaveResult(saveBtn, status, result); }); } diff --git a/packages/extension-shared/test/save-result-view.test.ts b/packages/extension-shared/test/save-result-view.test.ts new file mode 100644 index 0000000..27de2bb --- /dev/null +++ b/packages/extension-shared/test/save-result-view.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import type { Bookmark } from "@gitmarks/core"; +import { applySaveResult } from "../src/lib/save-result-view.js"; + +// applySaveResult ignores the bookmark payload; a cast keeps the fixture terse. +const okResult = { ok: true as const, bookmark: {} as Bookmark }; + +function makeDom(): { saveBtn: HTMLButtonElement; status: HTMLElement } { + const saveBtn = document.createElement("button"); + saveBtn.disabled = true; + saveBtn.textContent = "saving…"; + const status = document.createElement("p"); + return { saveBtn, status }; +} + +describe("applySaveResult", () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it("on success: shows saved, relabels button, marks it done, and auto-closes", () => { + const { saveBtn, status } = makeDom(); + const closePopup = vi.fn(); + + applySaveResult(saveBtn, status, okResult, { closePopup, closeDelayMs: 1200 }); + + expect(status.className).toBe("ok"); + expect(status.textContent).toBe("✓ saved"); + expect(saveBtn.textContent).toBe("Saved ✓"); + // No longer stuck on "saving…"; the `done` class cancels the progress cursor. + expect(saveBtn.classList.contains("done")).toBe(true); + + // Popup is not dismissed before the delay elapses... + expect(closePopup).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1200); + // ...and exactly once after. + expect(closePopup).toHaveBeenCalledTimes(1); + }); + + it("on failure: surfaces the message, re-enables the button, never closes", () => { + const { saveBtn, status } = makeDom(); + const closePopup = vi.fn(); + + applySaveResult( + saveBtn, + status, + { ok: false, kind: "unknown", message: "boom" }, + { closePopup }, + ); + + expect(status.className).toBe("err"); + expect(status.textContent).toBe("boom"); + expect(saveBtn.disabled).toBe(false); + expect(saveBtn.textContent).toBe("Try again"); + expect(saveBtn.classList.contains("done")).toBe(false); + + vi.advanceTimersByTime(10_000); + expect(closePopup).not.toHaveBeenCalled(); + }); +});