From 442f2a07ca1b4aeaa08b32c599aee66b402d181c Mon Sep 17 00:00:00 2001 From: Elian Doran Date: Sun, 11 Oct 2026 12:39:36 +0300 Subject: [PATCH] perf(clipper): read the whole page only when it is needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit parsed every page twice with Defuddle, once as an article and once as a whole, so that the Article / Whole page switch was instant. Most clips never use the switch, so every popup and every shortcut save paid for a parse whose result was thrown away. `readable-page.js` now reads one way per call. A file injection takes no arguments, so the script only registers `globalThis.triliumReadPage(variant)`, and `extractPage(tabId, variant)` calls it with a second `executeScript({ func, args })` in the same isolated world. Two script files would have bundled Defuddle twice. - The popup reads the article, and the whole page only when there is no article. Switching to Whole page reads it then, showing "Reading the page…" with Save disabled, and `PagePreview` keeps the result in `laterReadings` so switching back and forth reads it once. A reading is `undefined` until made and `null` when it shows nothing or fails, which disables its button and falls back to the article. - The shortcut and the context menu read the article, and the whole page only when there is none: one parse on an article page, as before the switch existed. `readActivePage()` falls back with consecutive `if`s rather than `article ?? await extractPage(...)`: written with `??`, v8 recorded a negative count for the code after it, and the branch gate failed with every case tested. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../skills/developing-web-clipper/SKILL.md | 33 +++++---- .../entrypoints/background/index.spec.ts | 19 ++--- .../entrypoints/background/index.ts | 11 ++- apps/web-clipper/entrypoints/popup/main.tsx | 74 +++++++++++++------ .../entrypoints/popup/popup.spec.tsx | 66 +++++++++++++---- .../entrypoints/readable-page/index.spec.ts | 30 ++++---- .../entrypoints/readable-page/index.ts | 45 +++++------ apps/web-clipper/utils.spec.ts | 27 ++++--- apps/web-clipper/utils.ts | 23 +++--- 9 files changed, 209 insertions(+), 119 deletions(-) diff --git a/.claude/skills/developing-web-clipper/SKILL.md b/.claude/skills/developing-web-clipper/SKILL.md index 2b36acef5f..22cd8bf007 100644 --- a/.claude/skills/developing-web-clipper/SKILL.md +++ b/.claude/skills/developing-web-clipper/SKILL.md @@ -18,7 +18,7 @@ repository. | `entrypoints/background/index.ts` | service worker (MV3) / background page (MV2) | context menus, keyboard commands, every save, image download, toasts | | `entrypoints/background/trilium_server_facade.ts` | background | finds Trilium, authenticates, `callService()` | | `entrypoints/content/index.ts` | every page (``) | selection, page description, crop overlay, toast UI | -| `entrypoints/readable-page/index.ts` | the tab, injected on demand | Defuddle extraction of the page, as an article and as a whole; its `main()` value is the `executeScript()` result | +| `entrypoints/readable-page/index.ts` | the tab, injected on demand | registers `triliumReadPage()`: Defuddle extraction of the page, as an article or as a whole | | `entrypoints/popup/main.tsx` | the toolbar popup | Preact `Popup`: capture buttons, link-with-note form, connection status | | `entrypoints/options/main.tsx` | the options page | Preact `Options`: desktop port, server login | | `entrypoints/offscreen/` | MV3 only (Chrome) | crops screenshots on a canvas, since an MV3 service worker has no DOM | @@ -38,11 +38,14 @@ Everything goes through `browser.runtime.sendMessage` / `browser.tabs.sendMessag (`message`, `noteId`, `tabIds`). - **background → offscreen:** `{ type: "CROP_IMAGE" }`, the one message keyed by `type`. -The readable page is not a message: the popup and the background call `extractPage(tabId)` -(`utils.ts`), which injects `/readable-page.js` with `browser.scripting.executeScript()` (the -`scripting` permission, in both builds), so Defuddle stays out of the content script that loads -on every page. Specs mock it with `Object.assign(fakeBrowser, { scripting: { executeScript } })`, -answering `[ { result: { article, wholePage } } ]` (`ExtractedPage`). +The readable page is not a message: the popup and the background call +`extractPage(tabId, variant)` (`utils.ts`), which injects `/readable-page.js` with +`browser.scripting.executeScript()` (the `scripting` permission, in both builds), so Defuddle stays +out of the content script that loads on every page. A file injection takes no arguments, so the +script only registers `globalThis.triliumReadPage()` and a second `executeScript({ func, args })` +calls it in the same isolated world. Specs mock it with +`Object.assign(fakeBrowser, { scripting: { executeScript } })`, answering `[]` to the file and +`[ { result: page } ]` to the call, by `args[0]`. Every user action in the background runs inside `showFailures()`, which turns an exception into a toast. A new action belongs inside it too, or it fails silently. A toast cannot show on browser @@ -120,16 +123,20 @@ against older versions of the other.** Prefer changes that need nothing new from made with `cloneNode()` does not have. `url` makes links and images absolute. - Its HTML is parsed with `DOMParser`, a document that loads no image and runs no code, before `getImages()` swaps the sources for placeholders. - - `readPage()` parses twice: the article, and the whole page (`WHOLE_PAGE_OPTIONS`: + - A page reads two ways: `"article"`, and `"wholePage"` (`WHOLE_PAGE_OPTIONS`: `contentSelector: "body"` with scoring and the clutter selectors off, hidden-element removal and standardization kept). Defuddle drops a grid of cards with little text under each heading as "related articles" (`isCardGrid()`) and any `.footer`, which on a gallery or test page is the - content (#9611). The popup offers both (`Article` / `Whole page`); the shortcut and the menu - save the article, or the whole page when there is none. - - A page without an article comes back as ``, which `readPage()` turns into - `article: undefined`; only a page whose whole page is empty too returns `undefined` (the - popup's bookmark mode). Defuddle's own retries relax its rules on a very short page ("possible - index page"), so a spec fixture of a card grid needs a paragraph of text to stay above that. + content (#9611). Each reading is one parse, made only when needed: the popup and the shortcut + read the article, and the whole page only when there is no article — or, in the popup, when + the user switches to `Whole page` (`laterReadings` in `PagePreview`). + - A reading that shows nothing (an article-less page comes back as ``) is + `undefined`; the popup stores it as `null`, keeping `undefined` for "not read yet". A page + whose two readings both show nothing opens on Bookmark. Defuddle's own retries relax its rules + on a very short page ("possible index page"), so a spec fixture of a card grid needs a + paragraph of text to stay above that. + - v8 can record a negative count for the code after `x ?? await y()`, which fails the branch + gate with every case tested; write such a fallback as consecutive `if`s. - Use `parse()`, never `parseAsync()`: the async extractors fetch third-party APIs. - **happy-dom drops lazy images** (`data-src` behind a `data:image/svg+xml` placeholder) that Chromium and linkedom promote to `src`; test such Defuddle behavior in a real browser, not diff --git a/apps/web-clipper/entrypoints/background/index.spec.ts b/apps/web-clipper/entrypoints/background/index.spec.ts index b069ab1576..93f7995c88 100644 --- a/apps/web-clipper/entrypoints/background/index.spec.ts +++ b/apps/web-clipper/entrypoints/background/index.spec.ts @@ -56,9 +56,9 @@ let tabs: Tab[]; let tabMessageHandler: (message: { name: string }) => unknown; const tabsSendMessage = vi.fn( (_tabId: number, message: { name: string }) => Promise.resolve(tabMessageHandler(message))); -/** What `readable-page.js` returns in the active tab; nothing to save unless a test says so. */ -let extractedPage: unknown; -const executeScript = vi.fn(async () => [ { result: extractedPage } ]); +/** How `readable-page.js` reads the active tab, by variant; nothing to save unless a test says so. */ +let pageReadings: Record; +const executeScript = vi.fn(async ({ args }: { args?: [ string ] }) => args ? [ { result: pageReadings[args[0]] } ] : []); const contextMenusCreate = vi.fn(); const contextMenusRemoveAll = vi.fn(async () => {}); const getContexts = vi.fn(async (): Promise => []); @@ -76,7 +76,7 @@ describe("background", () => { fakeBrowser.reset(); tabs = [ ACTIVE_TAB ]; tabMessageHandler = () => undefined; - extractedPage = undefined; + pageReadings = {}; facade.callService.mockResolvedValue({ noteId: "saved" }); vi.stubGlobal("fetch", fetchMock); vi.spyOn(console, "log").mockImplementation(() => {}); @@ -283,14 +283,15 @@ describe("background", () => { describe("saving the whole page", () => { it("saves the article from the shortcut, the menu and the popup", async () => { const page = { title: "Article", content: "

Body

", images: [], clipType: "page" }; - extractedPage = { article: page, wholePage: { ...page, content: "

Body

" } }; + pageReadings = { article: page, wholePage: { ...page, content: "

Body

" } }; await onCommand("saveWholePage"); await onContextMenuClicked({ menuItemId: "trilium-save-page" }); await sendRuntimeMessage({ name: "save-whole-page" }); - expect(executeScript).toHaveBeenCalledTimes(3); + expect(executeScript).toHaveBeenCalledTimes(6); expect(executeScript).toHaveBeenCalledWith({ target: { tabId: 7 }, files: [ "/readable-page.js" ] }); + expect(executeScript).not.toHaveBeenCalledWith(expect.objectContaining({ args: [ "wholePage" ] })); expect(facade.callService).toHaveBeenCalledTimes(3); expect(facade.callService).toHaveBeenLastCalledWith("POST", "notes", page); expect(await lastToast()).toMatchObject({ noteId: "saved" }); @@ -303,7 +304,7 @@ describe("background", () => { it("saves the whole page from the shortcut when the page has no article", async () => { const wholePage = { title: "Gallery", content: "

Card

", images: [], clipType: "page" }; - extractedPage = { wholePage }; + pageReadings = { wholePage }; await onCommand("saveWholePage"); @@ -318,7 +319,7 @@ describe("background", () => { message: "Saving to Trilium failed: Trilium found nothing to save on this page." }); - extractedPage = { wholePage: { title: "Article", content: "

Body

", images: [] } }; + pageReadings = { wholePage: { title: "Article", content: "

Body

", images: [] } }; tabs = [ { title: "Tab without id" } ]; await onCommand("saveWholePage"); @@ -330,7 +331,7 @@ describe("background", () => { message: "Saving to Trilium failed: Cannot access a chrome:// URL" }); - expect(executeScript).toHaveBeenCalledTimes(2); + expect(executeScript).toHaveBeenCalledTimes(5); expect(facade.callService).not.toHaveBeenCalled(); }); diff --git a/apps/web-clipper/entrypoints/background/index.ts b/apps/web-clipper/entrypoints/background/index.ts index 29bc141612..fd2f42c541 100644 --- a/apps/web-clipper/entrypoints/background/index.ts +++ b/apps/web-clipper/entrypoints/background/index.ts @@ -347,11 +347,14 @@ export default defineBackground(() => { async function readActivePage() { const activeTab = await getActiveTab(); - const page = activeTab.id === undefined ? undefined : await extractPage(activeTab.id); - if (!page) { - throw new Error("Trilium found nothing to save on this page."); + const tabId = activeTab.id; + if (tabId !== undefined) { + const article = await extractPage(tabId, "article"); + if (article) return article; + const wholePage = await extractPage(tabId, "wholePage"); + if (wholePage) return wholePage; } - return page.article ?? page.wholePage; + throw new Error("Trilium found nothing to save on this page."); } async function saveLinkWithNote(title: string, content: string, description?: string) { diff --git a/apps/web-clipper/entrypoints/popup/main.tsx b/apps/web-clipper/entrypoints/popup/main.tsx index 909a91cc9c..57b85537c5 100644 --- a/apps/web-clipper/entrypoints/popup/main.tsx +++ b/apps/web-clipper/entrypoints/popup/main.tsx @@ -24,7 +24,7 @@ import type { ComponentChildren } from "preact"; import { Fragment, render } from "preact"; import { useEffect, useRef, useState } from "preact/hooks"; -import { type ExtractedPage, extractPage, type ReadablePage } from "@/utils"; +import { extractPage, type PageVariant, type ReadablePage } from "@/utils"; import type { TriliumSearchNoteStatus, TriliumSearchStatus } from "../background/trilium_server_facade"; @@ -52,8 +52,11 @@ interface SelectedContent { labels?: Record; } -/** The current page: its article and its whole page, or why there is nothing to save. */ -type PageState = ExtractedPage | "unreadable" | "inaccessible"; +/** The tab, and each reading of its page: `undefined` until it is read, `null` when it shows nothing. */ +type PageReadings = { tabId: number } & Record; + +/** The current page: its readings, or why there is nothing to save. */ +type PageState = PageReadings | "unreadable" | "inaccessible"; /** What the active tab offers to save. */ interface TabContent { @@ -73,9 +76,6 @@ interface TabContent { */ type ClipMode = "selection" | "page" | "note"; -/** Which reading of the page the `page` mode saves. */ -type PageVariant = keyof ExtractedPage; - /** What the user has chosen and typed in the card, kept by `Popup` so a reconnection does not lose it. */ interface Draft { mode?: ClipMode; @@ -130,6 +130,12 @@ const MODE_LABELS: Record = { selection: "Selection", page: "P const VARIANT_LABELS: Record = { article: "Article", wholePage: "Whole page" }; +/** Why a reading of the page is unavailable. */ +const VARIANT_UNAVAILABLE: Record = { + article: "Trilium found no article on this page.", + wholePage: "Trilium found nothing to save on this page." +}; + /** The save button of each mode, with the command whose shortcut it shows, or its own shortcut. */ const SAVE_BUTTONS: Record = { selection: { icon: Highlight, label: "Save selection", command: "saveSelection" }, @@ -347,20 +353,33 @@ function PagePreview({ tab, disabled, shortcuts, draft, onDraftChange }: { }) { const { mode: chosenMode, variant: chosenVariant, title, description, author, text } = draft; const textAreaRef = useRef(null); + /** The readings of the page made after the popup opened, when the user switched to them. */ + const [ laterReadings, setLaterReadings ] = useState>({}); - const page = typeof tab?.page === "object" ? tab.page : undefined; - const variant = chosenVariant && page?.[chosenVariant] ? chosenVariant : page?.article ? "article" : "wholePage"; - const pageClip = page?.[variant]; + const page = typeof tab?.page === "object" ? { ...tab.page, ...laterReadings } : undefined; + const variant = chosenVariant && page?.[chosenVariant] !== null ? chosenVariant : page?.article ? "article" : "wholePage"; + const pageClip = page?.[variant] ?? undefined; + /** Either reading: both carry the same title and labels. */ + const pageReading = page?.article ?? page?.wholePage ?? undefined; const selection = tab?.selection ?? undefined; - const modes = tab ? availableModes(pageClip, selection) : []; + const modes = tab ? availableModes(!!page, selection) : []; const mode = chosenMode && modes.includes(chosenMode) ? chosenMode : modes[0]; const clip = mode === "selection" ? selection : mode === "page" ? pageClip : undefined; - const shownTitle = title ?? (pageClip ?? selection)?.title ?? tab?.title ?? ""; + const shownTitle = title ?? (pageReading ?? selection)?.title ?? tab?.title ?? ""; const shownDescription = description ?? tab?.description ?? ""; const savedDescription = shownDescription.replace(/\s+/g, " ").trim(); - const shownAuthor = author ?? pageClip?.labels.pageAuthor ?? ""; + const shownAuthor = author ?? pageReading?.labels.pageAuthor ?? ""; const savedAuthor = shownAuthor.replace(/\s+/g, " ").trim(); + const tabId = page?.tabId; + const unreadVariant = mode === "page" && page?.[variant] === undefined ? variant : undefined; + useEffect(() => { + if (tabId === undefined || !unreadVariant) return; + void readVariant(tabId, unreadVariant).then((reading) => { + setLaterReadings((readings) => ({ ...readings, [unreadVariant]: reading })); + }); + }, [ tabId, unreadVariant ]); + useEffect(() => { if (mode === "note") textAreaRef.current?.focus(); }, [ mode ]); @@ -404,7 +423,7 @@ function PagePreview({ tab, disabled, shortcuts, draft, onDraftChange }: { } const pageUrl = mode === "note" ? tab?.url : clip?.pageUrl; - const pageLabels = mode === "page" ? pageClip?.labels : undefined; + const pageLabels = mode === "page" ? pageReading?.labels : undefined; const published = pageLabels?.publishedDate; const modified = pageLabels?.modifiedDate; const button = SAVE_BUTTONS[mode ?? "page"]; @@ -521,8 +540,8 @@ function PagePreview({ tab, disabled, shortcuts, draft, onDraftChange }: {