diff --git a/AGENTS.md b/AGENTS.md index bad6f985..20336474 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,7 +199,11 @@ No need to explicitly document the telemetry behaviors. EVERY artifact→chrome message must go through `postArtifactMessage`, the only send path that stamps the current `artifact_load_token`: the chrome's message listener drops any frame message whose token is not the current load's before dispatch, so a raw `parent.postMessage` is silently discarded (a token-less `lavish:uploadAttachment` shipped exactly this break while every mocked harness stayed green - the chrome harness must therefore send test messages verbatim, never patch the token in). Nothing (Tailwind, DaisyUI, Mermaid, layout safety CSS) is auto-injected into artifacts; agents choose a design direction via the single-sourced `DESIGN_PRIORITY_RULE` (see AXI integration above). The `lavish-axi design` Mermaid snippet chooses dark or light rendering from the effective artifact page background and re-renders when a page-theme or OS appearance change alters that appearance, so do not hardcode one Mermaid theme. -- For rendered Mermaid SVGs outside `.mermaid` containers, the injected SDK retains dependency-free viewBox pan (drag) and zoom (wheel) in explore mode, then freezes it in annotation mode so a click resolves cleanly to one node instead of panning. It enhances on load and `DOMContentLoaded` and re-runs through a throttled `MutationObserver` because Mermaid renders asynchronously and can re-render. Enhancement touches only the live SVG's `viewBox` and listeners, never the saved artifact, so the diagram still renders identically when opened directly. Node detection, label extraction, and target validation live in `src/mermaid-node.js` so they are unit-testable and shared with the server; `createSdkJs` serializes each exported helper into the SDK as a same-scope `const` (like `deriveQueueKey`), derived from the module's exports, so a helper may reference only its own arguments, browser globals, or its sibling exports. +- For rendered Mermaid SVGs outside `.mermaid` containers, the injected SDK retains dependency-free viewBox pan (drag) and zoom (wheel) in explore mode, then freezes it in annotation mode so a click resolves cleanly to one node instead of panning. It enhances on load and `DOMContentLoaded` and re-runs through a throttled `MutationObserver` because Mermaid renders asynchronously and can re-render. Enhancement touches only the live SVG's `viewBox` and listeners, never the saved artifact, so the diagram still renders identically when opened directly. Node detection, label extraction, and target validation live in `src/mermaid-node.js` so they are unit-testable and shared with the server. +- Any helper `createArtifactSdk` calls must reach the browser through `serializeModuleHelpers` in `createSdkJs`, which turns every export of a shared module (`src/mermaid-node.js`, `src/table-cell.js`) into a same-scope `const`. A module-private function called from the SDK closure compiles fine and only `ReferenceError`s on the first click, so put new helpers in one of those wholesale-serialized modules and export them; a helper may then reference only its own arguments, browser globals, or its sibling exports - never a module-level constant, which is not serialized. Those modules must export functions and nothing else: only functions survive `toString()`, so `serializeModuleHelpers` throws on any other export rather than shipping a `Set` or `RegExp` that would arrive as an empty `{}`. `test/artifact-sdk-bundle.test.js` boots the served bundle and drives a real click, which is what catches an unreachable helper; the module-level unit tests cannot. +- Table-cell annotations attach `src/table-cell.js`'s semantic row/column names as `target` only. The clicked element's own `selector`, `tag`, and `text` keep describing that element, because the on-screen highlight outlines exactly what was clicked. Both coordinates stay silent rather than name a row or column they cannot prove, because a confidently wrong name reads as authoritative and is worse than none: a rowspan is clipped to its own row group, so only one starting in an earlier row of that group (including `rowspan="0"`, which runs to the end of it) makes a row's DOM order stop being its rendered order, and that suppresses the row's positional heading - only a declared `scope="row"` heading survives it - while the column label needs the header row unshifted the same way, plus a row whose colspans sum to the header's and a cell that does not straddle a grouped header. + A grouped header's `` therefore costs the leaf header row its names and leaves every `` row nameable. + Spans come from the browser-parsed `rowSpan`/`colSpan` whenever present, because HTML's integer rules stop at the first non-digit and render `rowspan="2x"` as a real two-row span that `Number` reads as `NaN`. Resolving a cell walks its whole table, so `context()` computes the target only under `{ table: true }`, which the annotation card passes and `snapshot()` - which calls `context()` for every element in the document - deliberately does not. - Annotation handlers ignore native controls (`button`, `input`, `select`, `textarea`, `option`, `label`, `summary`, and editable regions) and their descendants, so they stay interactive without markup; `data-lavish-action` opts custom non-native controls out of annotation and gives them a pointer cursor. Artifact-author guidance for input patterns (`data-lavish-question`, `queueKey`, per-question submits) is owned by the input playbook in `src/playbooks.js`. - For text annotations, `prompt.selector` is the common ancestor/container selector, not the complete identity. Use the `target` range boundaries and snapshot context to locate the exact selected text. - For non-whiteboard Mermaid diagram nodes, a click annotates the whole rendered `` node - not the sub-shape under the cursor - and hover highlights the same node. `SessionStore.normalizeTarget` routes these targets through `normalizeMermaidNodeTarget`, which strips them to the fixed `type`/`diagramId`/`nodeId`/`label`/`selector` shape, while text-range and other/legacy targets pass through unchanged. diff --git a/README.md b/README.md index efc9a16c..7c605dba 100644 --- a/README.md +++ b/README.md @@ -202,7 +202,10 @@ pnpm link A plain `lavish-axi ` after a user-initiated end refuses to reopen the browser and returns guidance instead; pass `--reopen` only when the user asks for further review or something important needs their visual attention. Agent-initiated ends keep reopening normally, same as before. `lavish-axi poll`'s `ended` response and the `feedback` response for the final batch before an end both carry `next_step` guidance telling the agent to stop polling and deliver remaining updates in chat instead of reopening. -- **Precise targets** - Text annotations include selected text plus range anchors, so agents are not limited to whole-element selectors. +- **Precise targets** - Text annotations include selected text plus range anchors, and text selections carry those anchors only. + Clicking an element inside a table also carries the cell's visible row and column names alongside the exact CSS locator, so filtered or sorted rows do not make feedback look misdirected. + When merged cells make either name ambiguous, Lavish leaves that name out rather than guessing; an explicit `` remains authoritative even when a `rowspan` makes the row's position ambiguous. + The CSS locator still points at the exact element you clicked, so an annotation with an omitted name is only less descriptive, never mislabelled. - **Image attachments** - Attach reference images (PNG, JPEG, WebP) to an annotation by pasting, drag-dropping, or using the annotation card's **Attach image** picker; each shows a thumbnail chip with upload, remove, retry, and error states. Images are stored under the state dir and the queued prompt carries a server-generated absolute `path` and content-hash `id` (plus mime and dimensions) - never the raw bytes - so `lavish-axi poll` hands the agent a local file path to open. Limits are `LAVISH_AXI_MAX_ATTACHMENT_BYTES` (default 10 MiB per image), `LAVISH_AXI_MAX_ATTACHMENTS_PER_PROMPT` (default 4), and `LAVISH_AXI_MAX_PROMPT_ATTACHMENT_BYTES` (default 25 MiB per annotation); if any image is missing or any annotation breaches a count or byte cap, the entire send batch is rejected, the queue is preserved, and the reason is surfaced in the composer rather than silently dropping images. diff --git a/src/artifact-sdk.js b/src/artifact-sdk.js index b3067f3f..f6179af4 100644 --- a/src/artifact-sdk.js +++ b/src/artifact-sdk.js @@ -1,6 +1,7 @@ /* global CSS, Element, MutationObserver, ResizeObserver, document, getComputedStyle, parent, window */ import * as mermaidHelpers from "./mermaid-node.js"; +import { tableCellTarget } from "./table-cell.js"; export const LAVISH_INTERNAL_QUEUE_KEY = "_lavishQueueKey"; @@ -746,7 +747,9 @@ export function createArtifactSdk( return parts.join(" > "); } - function context(el) { + // `table` is opt-in because resolving a cell's row and column walks the whole table, while + // `snapshot()` calls this for every element in the document and reads only uid/tag/text. + function context(el, { table = false } = {}) { const base = { uid: uid(el), selector: selector(el), @@ -754,6 +757,12 @@ export function createArtifactSdk( text: (el.innerText || el.textContent || "").trim().replace(/\s+/g, " ").slice(0, 240), }; + // Semantic table coordinates are extra context, never a replacement identity: the highlight + // outlines the element the reviewer clicked, so its selector, tag, and text must keep + // describing that exact element rather than being coarsened up to the enclosing cell. + const tableTarget = table ? tableCellTarget(el, selector) : null; + if (tableTarget) base.target = tableTarget; + const mermaidNode = mermaidNodeFrom(el, selector); if (mermaidNode) { base.tag = "mermaid-node"; @@ -2131,7 +2140,7 @@ export function createArtifactSdk( const root = ensureShadow(); closeCard(); - const c = options.context || context(target); + const c = options.context || context(target, { table: true }); activeCardContext = c; let anchor = target; if (options.range) { @@ -2146,18 +2155,31 @@ export function createArtifactSdk( const card = document.createElement("div"); card.className = "lavish-annotation-card"; const nodeLabel = c.tag === "mermaid-node" ? c.target?.label || c.text || "" : ""; + const isTableCell = c.target?.type === "table-cell"; + // The annotation targets the element that was clicked, which inside a table cell is often a + // nested badge or code span. Say "cell" only when the cell itself was clicked; otherwise name + // the clicked element and place it at the cell's coordinates. An unlabelled table names + // nothing, so it falls back to the plain element heading rather than a dangling "cell: ". + const isCellItself = isTableCell && (c.tag === "td" || c.tag === "th"); + const tableLabel = isTableCell ? [c.target?.rowLabel, c.target?.columnLabel].filter(Boolean).join(" → ") : ""; const heading = c.tag === "text" ? "Annotate text" - : c.tag === "mermaid-node" - ? "Annotate node" + (nodeLabel ? ": " + escapeAnnotationText(nodeLabel) : "") - : "Annotate <" + c.tag + ">"; + : tableLabel + ? isCellItself + ? "Annotate cell: " + escapeAnnotationText(tableLabel) + : "Annotate <" + c.tag + "> in " + escapeAnnotationText(tableLabel) + : c.tag === "mermaid-node" + ? "Annotate node" + (nodeLabel ? ": " + escapeAnnotationText(nodeLabel) : "") + : "Annotate <" + c.tag + ">"; const placeholder = c.tag === "text" ? "Tell the agent what to change about this text..." - : c.tag === "mermaid-node" - ? "Tell the agent what to change about this diagram node..." - : "Tell the agent what to change about this element..."; + : isCellItself + ? "Tell the agent what to change about this table cell..." + : c.tag === "mermaid-node" + ? "Tell the agent what to change about this diagram node..." + : "Tell the agent what to change about this element..."; const sendNowHint = /Mac|iP(hone|ad|od)/.test(navigator.platform) ? "⌘" : "Ctrl"; card.innerHTML = '
' + diff --git a/src/chrome-client.js b/src/chrome-client.js index afbf96c6..acd478b9 100644 --- a/src/chrome-client.js +++ b/src/chrome-client.js @@ -277,10 +277,20 @@ function persistQueuedPrompts() { } } +function promptTargetLabel(prompt) { + if (prompt?.target?.type === "table-cell") { + const semantic = [prompt.target.rowLabel, prompt.target.columnLabel].filter(Boolean).join(" → "); + if (semantic) return semantic; + } + return String(prompt?.selector || ""); +} + function render() { annotationPills.innerHTML = queued - .map( - (prompt, index) => + .map((prompt, index) => { + const targetLabel = promptTargetLabel(prompt); + const showLocator = targetLabel && prompt.selector && targetLabel !== prompt.selector; + return ( '
' + escapeHtml(prompt.prompt || (attachmentCount(prompt) ? "Image annotation" : "")) + "" + @@ -288,15 +298,21 @@ function render() { '
' + - (prompt.selector + (targetLabel ? '
Target
' + + escapeHtml(targetLabel) + + "
" + : "") + + (showLocator + ? '
Locator
' + escapeHtml(prompt.selector) + "
" : "") + '
Prompt
' + escapeHtml(prompt.prompt) + - "
", - ) + "
" + ); + }) .join(""); for (const button of annotationPills.querySelectorAll(".pill-close")) { diff --git a/src/mermaid-node.js b/src/mermaid-node.js index 51d45931..b6d75218 100644 --- a/src/mermaid-node.js +++ b/src/mermaid-node.js @@ -2,10 +2,10 @@ // Pure Mermaid node-identity helpers shared by the injected artifact SDK and the // server-side session store. The SDK ships them to the browser by serializing -// each one with `.toString()` (see `createSdkJs`), which drops the surrounding -// module scope — so a helper may reference only its own arguments, browser -// globals, or its sibling exports from this module. `createSdkJs` re-declares -// every export here as a same-scope `const` before invoking the SDK, so +// each one with `.toString()` via `serializeModuleHelpers` in `createSdkJs`, which +// drops the surrounding module scope — so a helper may reference only its own +// arguments, browser globals, or its sibling exports from this module. `createSdkJs` +// re-declares every export here as a same-scope `const` before invoking the SDK, so // cross-helper calls (e.g. `mermaidNodeFrom` → `mermaidNodeElement`) resolve in // the browser exactly as they do here; never close over anything else. Keeping // the logic here — instead of inside the `createArtifactSdk` closure — lets us diff --git a/src/server.js b/src/server.js index 10c2b425..49b76d41 100644 --- a/src/server.js +++ b/src/server.js @@ -33,6 +33,7 @@ import { serializeLayoutWarnings, } from "./layout-warnings.js"; import * as mermaidNode from "./mermaid-node.js"; +import * as tableCellHelpers from "./table-cell.js"; import { extractMermaidSources, mermaidSourceHash } from "./mermaid-source.js"; import { isValidDiagramIndex, @@ -1889,6 +1890,27 @@ export function createWhiteboardFrameHtml(channelToken = "") { `; } +// Serialize every helper a shared module exports as a same-scope const so cross-helper calls +// (e.g. mermaidNodeFrom → mermaidNodeElement) resolve in the browser. Deriving these from the +// module's exports — rather than a hand-kept list — means adding a helper can never silently +// ReferenceError at runtime. +// Only functions survive `toString()` round-tripping: a Set, Map, or RegExp would serialize to a +// valid-looking `{}` and reach the browser semantically empty, which is far harder to find than +// this throw. A shared module must therefore export nothing but helpers. +function serializeModuleHelpers(module) { + const entries = Object.entries(module); + const unsupported = entries.filter(([, value]) => typeof value !== "function").map(([name]) => name); + if (unsupported.length > 0) { + throw new TypeError( + `Cannot serialize non-function SDK helper export(s) into the artifact bundle: ${unsupported.join(", ")}`, + ); + } + return { + declarations: entries.map(([name, fn]) => `const ${name}=${fn.toString()};`).join("\n"), + names: entries.map(([name]) => name), + }; +} + /** * @param {string} key * @param {number} [artifactRevision] @@ -1901,13 +1923,8 @@ export function createSdkJs( artifactLoadToken = "", { maxAttachmentCount, maxAttachmentBytes } = {}, ) { - // Serialize every helper exported by mermaid-node.js as a same-scope const so - // cross-helper calls (e.g. mermaidNodeFrom → mermaidNodeElement) resolve in the - // browser. Deriving this from the module's exports — rather than a hand-kept - // list — means adding a helper can never silently ReferenceError at runtime. - const mermaidHelperEntries = Object.entries(mermaidNode).filter(([, value]) => typeof value === "function"); - const mermaidHelperDecls = mermaidHelperEntries.map(([name, fn]) => `const ${name}=${fn.toString()};`).join("\n"); - const mermaidHelperKeys = mermaidHelperEntries.map(([name]) => name).join(", "); + const mermaidHelperSource = serializeModuleHelpers(mermaidNode); + const tableHelperSource = serializeModuleHelpers(tableCellHelpers); const revisionNumber = Number(artifactRevision); const revision = Number.isFinite(revisionNumber) && revisionNumber >= 0 ? Math.trunc(revisionNumber) : 0; const loadToken = String(artifactLoadToken || "").slice(0, 200); @@ -1937,8 +1954,9 @@ const classifyAttachmentBatch=${classifyAttachmentBatch.toString()}; const partitionDroppedFiles=${partitionDroppedFiles.toString()}; const isTrustedAttachmentResult=${isTrustedAttachmentResult.toString()}; const deriveAttachmentNoticeState=${deriveAttachmentNoticeState.toString()}; -${mermaidHelperDecls} -const mermaidHelpers={ ${mermaidHelperKeys} }; +${mermaidHelperSource.declarations} +const mermaidHelpers={ ${mermaidHelperSource.names.join(", ")} }; +${tableHelperSource.declarations} (${createArtifactSdk.toString()})(deriveQueueKey, isNativeInteractiveControl, mermaidHelpers, artifactRevision, artifactLoadToken, key, ${JSON.stringify(sdkOptions)}); })();`; } diff --git a/src/table-cell.js b/src/table-cell.js new file mode 100644 index 00000000..3337d26c --- /dev/null +++ b/src/table-cell.js @@ -0,0 +1,168 @@ +// Semantic naming for annotated table cells. Positional selectors stay the locator, but a +// filtered or sorted table makes their row numbers read wrong to a reviewer, so an annotation +// also carries the visible row and column names. +// +// Every helper here is serialized wholesale into the artifact SDK bundle by `createSdkJs`, so +// each one may reference only its own arguments, browser globals, or its sibling exports. + +export function tableTagName(element) { + return String(element?.tagName || element?.nodeName || "").toLowerCase(); +} + +export function tableText(element) { + return String(element?.innerText || element?.textContent || "") + .trim() + .replace(/\s+/g, " ") + .slice(0, 240); +} + +// Rows of one table only. Descending into a cell would both walk the whole document subtree and +// collect a nested table's rows, which would then be read as this table's header or spans. +export function tableRowsIn(element) { + const rows = []; + for (const child of Array.from(element?.children || [])) { + const tag = tableTagName(child); + if (tag === "td" || tag === "th" || tag === "table") continue; + if (tag === "tr") rows.push(child); + else rows.push(...tableRowsIn(child)); + } + return rows; +} + +export function tableRowCells(row) { + return Array.from(row?.children || []).filter((cell) => { + const tag = tableTagName(cell); + return tag === "td" || tag === "th"; + }); +} + +// The attribute string is not the rendered span. HTML's rules for parsing a non-negative integer +// stop at the first non-digit, so `rowspan="2x"` really does span two rows even though `Number` +// reads it as `NaN` - reporting no span there would emit labels for a grid that is actually +// shifted. Take the span the browser already parsed whenever there is one, parse the attribute the +// same way when there is not, and let a value that parses to nothing fall back to the attribute's +// default, exactly as a browser does. +export function tableSpanValue(cell, name, parsed) { + if (typeof parsed === "number" && Number.isInteger(parsed) && parsed >= 0) return parsed; + const digits = /^[\t\n\f\r ]*(\d+)/.exec(String(cell?.getAttribute?.(name) ?? "")); + return digits ? Number(digits[1]) : null; +} + +export function tableColumnSpan(cell) { + const span = tableSpanValue(cell, "colspan", cell?.colSpan); + return span !== null && span >= 1 ? span : 1; +} + +// `rowspan="0"` is valid HTML - it spans to the end of the row group - and browsers report +// `cell.rowSpan === 0` for it. A finite span shifts only the following rows it actually reaches. +export function tableCellSpansRows(cell, rowDistance = 1) { + const span = tableSpanValue(cell, "rowspan", cell?.rowSpan); + const renderedSpan = span === null || span < 0 ? 1 : span; + return renderedSpan === 0 || renderedSpan > rowDistance; +} + +// A rowspan is clipped to its own row group, so the group is the widest span of rows one can +// shift. Rows outside it - a `` grouped header in ``, say - leave this row +// laid out from column 0. +export function tableRowGroup(table, row) { + let ancestor = row?.parentElement || null; + while (ancestor && ancestor !== table) { + const tag = tableTagName(ancestor); + if (tag === "thead" || tag === "tbody" || tag === "tfoot") return ancestor; + ancestor = ancestor.parentElement; + } + return table; +} + +// A span starting in an earlier row of this row's own group means its DOM order is no longer its +// rendered column order, and a per-row walk cannot model that. A finite span shifts only rows +// within its declared range, while rowspan=0 reaches the end of the group; a row that cannot be +// placed in its own group at all is unprovable the same way. +export function tableRowIsShifted(table, row) { + if (!table || !row) return true; + const rows = tableRowsIn(tableRowGroup(table, row)); + const index = rows.indexOf(row); + if (index < 0) return true; + for (let i = 0; i < index; i += 1) { + for (const cell of tableRowCells(rows[i])) { + if (tableCellSpansRows(cell, index - i)) return true; + } + } + return false; +} + +// Browsers auto-insert but never , so a hand-written table commonly keeps its +// header cells in the first . Adopt that row only when it is unambiguously a header - every +// cell a - rather than guessing that the first data row names the columns. +export function tableHeaderRow(table) { + const head = Array.from(table?.children || []).find((child) => tableTagName(child) === "thead"); + if (head) return tableRowsIn(head).at(-1) || null; + const first = tableRowsIn(table)[0]; + const cells = tableRowCells(first); + return cells.length > 0 && cells.every((cell) => tableTagName(cell) === "th") ? first : null; +} + +// A confidently wrong column name reads as authoritative and is worse than none, so this returns +// a label only when the clicked cell's grid range provably matches exactly one header cell: a row +// whose spans do not sum to the header's is not the same grid, and a cell straddling a grouped +// header names nothing. The caller rules out rowspan-shifted grids before calling. +export function tableColumnLabel(headerRow, cells, index) { + if (!headerRow) return ""; + const headerCells = tableRowCells(headerRow); + const width = (cell) => tableColumnSpan(cell); + const headerWidth = headerCells.reduce((sum, cell) => sum + width(cell), 0); + const rowWidth = cells.reduce((sum, cell) => sum + width(cell), 0); + if (headerWidth === 0 || headerWidth !== rowWidth) return ""; + + let start = 0; + for (let i = 0; i < index; i += 1) start += width(cells[i]); + const end = start + width(cells[index]); + + let cursor = 0; + for (const header of headerCells) { + const next = cursor + width(header); + if (cursor === start && next === end) return tableText(header); + if (start < next) return ""; + cursor = next; + } + return ""; +} + +export function tableCellTarget(element, selectorFor = (_element) => "") { + const cell = element?.closest?.("td,th"); + const row = cell?.closest?.("tr"); + const table = row?.closest?.("table"); + if (!cell || !row || !table) return null; + + const cells = tableRowCells(row); + const index = cells.indexOf(cell); + if (index < 0) return null; + + const headerRow = tableHeaderRow(table); + const shifted = tableRowIsShifted(table, row); + // A shifted header row cannot name columns either, even when the clicked row is laid out + // straight: its own cells are no longer in column order, so nothing can be matched against them. + const gridShifted = shifted || (headerRow ? tableRowIsShifted(table, headerRow) : false); + const declaredHeading = cells.find( + (candidate) => + tableTagName(candidate) === "th" && String(candidate.getAttribute?.("scope") || "").toLowerCase() === "row", + ); + // No header row names a record, so none of them gets a row label - not just the one row + // `tableHeaderRow` picks. In a grouped header the first cell of any other header row is a + // sibling column header, and naming the click after it reads as a row name that does not exist. + // A row of nothing but `th` is that signal without a ``, which browsers never insert. + // `scope="row"` is an author declaration and outranks both this and a shifted row, but taking + // the first DOM cell is a positional guess that a rowspan reaching into this row invalidates - + // it can even name the clicked cell after itself. + const allHeaderCells = cells.every((candidate) => tableTagName(candidate) === "th"); + const inHeaderSection = headerRow === row || Boolean(cell.closest?.("thead")); + const rowHeading = inHeaderSection ? null : declaredHeading || (allHeaderCells || shifted ? null : cells[0]); + + return { + type: "table-cell", + selector: String(selectorFor(cell) || "").slice(0, 240), + rowLabel: tableText(rowHeading), + columnLabel: gridShifted ? "" : tableColumnLabel(headerRow, cells, index), + text: tableText(cell), + }; +} diff --git a/test/artifact-sdk-bundle.test.js b/test/artifact-sdk-bundle.test.js new file mode 100644 index 00000000..751b490f --- /dev/null +++ b/test/artifact-sdk-bundle.test.js @@ -0,0 +1,286 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import vm from "node:vm"; + +import { createSdkJs } from "../src/server.js"; + +// The SDK the browser actually runs is a serialized bundle, not the module: `createSdkJs` has to +// declare every helper `createArtifactSdk` reaches for. A helper left out compiles fine and only +// ReferenceErrors on the first click, so these tests boot the served bundle and drive the real +// annotation path through a DOM stub instead of inspecting the module directly. + +function createElement(tag) { + const attributes = new Map(); + const queried = new Map(); + const element = { + tagName: String(tag).toUpperCase(), + nodeName: String(tag).toUpperCase(), + nodeType: 1, + parentElement: null, + children: [], + style: {}, + value: "", + innerHTML: "", + textContent: "", + offsetWidth: 100, + offsetHeight: 100, + hidden: false, + listeners: [], + classList: { + add() {}, + remove() {}, + contains() { + return false; + }, + }, + setAttribute(name, value) { + attributes.set(name, String(value)); + }, + getAttribute(name) { + return attributes.has(name) ? attributes.get(name) : null; + }, + matches(selectorList) { + return String(selectorList) + .split(",") + .some((part) => { + const selector = part.trim(); + if (selector.startsWith("[")) return attributes.has(selector.slice(1, selector.indexOf("]")).split("=")[0]); + return selector === element.tagName.toLowerCase(); + }); + }, + closest(selectorList) { + let current = element; + while (current) { + if (current.matches(selectorList)) return current; + current = current.parentElement; + } + return null; + }, + appendChild(child) { + child.parentElement = element; + element.children.push(child); + return child; + }, + remove() { + const index = element.parentElement?.children.indexOf(element) ?? -1; + if (index >= 0) element.parentElement.children.splice(index, 1); + }, + // Card internals are looked up by class after innerHTML is assigned, so hand back a stable + // stub per selector: the test drives the very buttons the SDK wired up. + querySelector(selector) { + if (!queried.has(selector)) queried.set(selector, createElement(selector.replace(/^[.#]/, ""))); + return queried.get(selector); + }, + querySelectorAll() { + return []; + }, + getBoundingClientRect() { + return { left: 10, top: 10, right: 110, bottom: 40, width: 100, height: 30 }; + }, + addEventListener(type, handler) { + element.listeners.push({ type, handler }); + }, + removeEventListener() {}, + focus() {}, + click() {}, + scrollIntoView() {}, + attachShadow() { + element.shadowRoot = createElement("shadow-root"); + return element.shadowRoot; + }, + }; + return element; +} + +function appendTo(parent, child) { + child.parentElement = parent; + parent.children.push(child); + return child; +} + +function cell(tag, text) { + const element = createElement(tag); + element.textContent = text; + return element; +} + +function bootSdk() { + const posted = []; + const documentListeners = []; + const documentElement = createElement("html"); + const head = createElement("head"); + const body = createElement("body"); + appendTo(documentElement, head); + appendTo(documentElement, body); + + const sandbox = { + parent: { postMessage: (message) => posted.push(message) }, + navigator: { platform: "Linux" }, + CSS: { escape: (value) => String(value) }, + Element: class Element {}, + MutationObserver: class MutationObserver { + observe() {} + disconnect() {} + }, + ResizeObserver: class ResizeObserver { + observe() {} + disconnect() {} + }, + URL: { + createObjectURL() { + return "blob:lavish-test"; + }, + revokeObjectURL() {}, + }, + getComputedStyle: () => ({}), + setTimeout: () => 0, + clearTimeout() {}, + requestAnimationFrame: () => 0, + document: { + readyState: "complete", + documentElement, + head, + body, + activeElement: body, + baseURI: "http://127.0.0.1/artifact/abc/index.html", + addEventListener: (type, handler) => documentListeners.push({ type, handler }), + removeEventListener() {}, + createElement, + getElementById: () => null, + querySelector: () => null, + querySelectorAll: () => [], + getSelection: () => null, + }, + }; + sandbox.window = { + addEventListener() {}, + removeEventListener() {}, + setTimeout: () => 0, + clearTimeout() {}, + requestAnimationFrame: () => 0, + innerWidth: 1280, + innerHeight: 800, + scrollX: 0, + scrollY: 0, + location: { origin: "http://127.0.0.1" }, + URL: sandbox.URL, + }; + sandbox.globalThis = sandbox; + + vm.runInNewContext(createSdkJs("abc", 3, "load-token"), sandbox); + + return { + posted, + body, + api: sandbox.window.lavish, + click(target) { + const listener = documentListeners.find((entry) => entry.type === "click"); + assert.ok(listener, "the SDK registers a document click listener"); + listener.handler({ target, preventDefault() {}, stopPropagation() {} }); + }, + card() { + const card = documentElement.children + .flatMap((child) => child.shadowRoot?.children || []) + .findLast((child) => child.className === "lavish-annotation-card"); + assert.ok(card, "clicking an element opens an annotation card"); + return card; + }, + queue(text) { + const card = this.card(); + card.querySelector("textarea").value = text; + card.querySelector(".lavish-send").onclick(); + return posted.at(-1); + }, + }; +} + +function buildTable(sdk) { + const table = appendTo(sdk.body, createElement("table")); + const thead = appendTo(table, createElement("thead")); + const headerRow = appendTo(thead, createElement("tr")); + for (const label of ["Permission / setting", "Visible state", "Database evidence"]) { + appendTo(headerRow, cell("th", label)); + } + const tbody = appendTo(table, createElement("tbody")); + const dataRow = appendTo(tbody, createElement("tr")); + appendTo(dataRow, cell("td", "Media & Apple Music")); + appendTo(dataRow, cell("td", "4 apps")); + const evidence = appendTo(dataRow, cell("td", "Drive, Neovide, Cursor")); + const badge = appendTo(evidence, cell("code", "Drive")); + return { evidence, badge }; +} + +test("the served SDK bundle queues a table-cell annotation without a missing-helper ReferenceError", () => { + const sdk = bootSdk(); + const { evidence } = buildTable(sdk); + + sdk.click(evidence); + const message = sdk.queue("Check this permission"); + + assert.equal(message.type, "lavish:queuePrompt"); + assert.equal(message.prompt.prompt, "Check this permission"); + assert.deepEqual( + { ...message.prompt.target }, + { + type: "table-cell", + selector: "body > table > tbody > tr > td:nth-of-type(3)", + rowLabel: "Media & Apple Music", + columnLabel: "Database evidence", + text: "Drive, Neovide, Cursor", + }, + ); +}); + +test("the served SDK bundle keeps the clicked element's own identity inside a table cell", () => { + const sdk = bootSdk(); + const { badge } = buildTable(sdk); + + sdk.click(badge); + const message = sdk.queue("Rename this app"); + + assert.equal(message.prompt.tag, "code"); + assert.equal(message.prompt.selector, "table > tbody > tr > td:nth-of-type(3) > code"); + assert.equal(message.prompt.text, "Drive"); + assert.equal(message.prompt.target.selector, "body > table > tbody > tr > td:nth-of-type(3)"); + assert.equal(message.prompt.target.columnLabel, "Database evidence"); +}); + +test("the annotation card names the cell it annotates when the cell itself is clicked", () => { + const sdk = bootSdk(); + const { evidence } = buildTable(sdk); + + sdk.click(evidence); + + assert.match(sdk.card().innerHTML, /Annotate cell: Media & Apple Music → Database evidence/); + assert.match(sdk.card().innerHTML, /about this table cell/); +}); + +test("the annotation card names the clicked element, not the cell, for a nested click", () => { + const sdk = bootSdk(); + const { badge } = buildTable(sdk); + + sdk.click(badge); + + assert.match(sdk.card().innerHTML, /Annotate <code> in Media & Apple Music → Database evidence/); + assert.doesNotMatch(sdk.card().innerHTML, /about this table cell/); +}); + +test("the served SDK bundle resolves table coordinates only for annotation clicks", () => { + const sdk = bootSdk(); + const { evidence } = buildTable(sdk); + + sdk.api.queuePrompt("Programmatic note", { element: evidence }); + + assert.equal(sdk.posted.at(-1).prompt.target, undefined); +}); + +test("the served SDK bundle annotates elements outside tables with no table target", () => { + const sdk = bootSdk(); + const paragraph = appendTo(sdk.body, cell("p", "Just prose")); + + sdk.click(paragraph); + const message = sdk.queue("Reword this"); + + assert.equal(message.prompt.tag, "p"); + assert.equal(message.prompt.target, undefined); +}); diff --git a/test/chrome-client-queue.test.js b/test/chrome-client-queue.test.js index 943b98dd..33db80fa 100644 --- a/test/chrome-client-queue.test.js +++ b/test/chrome-client-queue.test.js @@ -542,6 +542,49 @@ test("chrome client replaces queued prompts with the same internal key", async ( assert.doesNotMatch(chrome.element("annotationPills").innerHTML, /Use plan A/); }); +test("chrome client shows semantic table coordinates before positional selector", async () => { + const chrome = await createChromeHarness(); + + chrome.sendFrameMessage({ + type: "lavish:queuePrompt", + prompt: { + prompt: "Check this permission", + selector: "table > tbody > tr:nth-of-type(7) > td:nth-of-type(3) > code", + tag: "code", + text: "Drive", + target: { + type: "table-cell", + selector: "table > tbody > tr:nth-of-type(7) > td:nth-of-type(3)", + rowLabel: "Media & Apple Music", + columnLabel: "Database evidence", + text: "Drive, Neovide, Cursor, Alacritty", + }, + }, + }); + + assert.match(chrome.element("annotationPills").innerHTML, /Media & Apple Music → Database evidence/); + assert.match(chrome.element("annotationPills").innerHTML, /tr:nth-of-type\(7\)/); +}); + +test("chrome client falls back to the locator when a table cell has no row or column name", async () => { + const chrome = await createChromeHarness(); + + chrome.sendFrameMessage({ + type: "lavish:queuePrompt", + prompt: { + prompt: "Check this permission", + selector: "table > tbody > tr:nth-of-type(7) > td:nth-of-type(3)", + tag: "td", + text: "Drive", + target: { type: "table-cell", rowLabel: "", columnLabel: "", text: "Drive" }, + }, + }); + + const html = chrome.element("annotationPills").innerHTML; + assert.match(html, /tr:nth-of-type\(7\)/); + assert.doesNotMatch(html, /Locator/); +}); + test("chrome client scrolls new chat bubbles into view above queued prompts", async () => { const chrome = await createChromeHarness(); const panelScroll = chrome.element("panelScroll"); diff --git a/test/server.test.js b/test/server.test.js index 7e196ac8..e3c70a41 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -326,6 +326,14 @@ test("artifact SDK injects every shared mermaid node helper as a same-scope cons assert.match(js, /const mermaidHelpers=\{[^}]*mermaidNodeElement[^}]*\}/); }); +test("shared SDK helper modules export only functions so serializeModuleHelpers can ship them", async () => { + const mermaid = await import("../src/mermaid-node.js"); + const table = await import("../src/table-cell.js"); + for (const [name, value] of [...Object.entries(mermaid), ...Object.entries(table)]) { + assert.equal(typeof value, "function", `${name} must be a function`); + } +}); + test("annotation hover and click resolve to the same Mermaid node element", () => { const js = createSdkJs("abc"); diff --git a/test/table-cell.test.js b/test/table-cell.test.js new file mode 100644 index 00000000..f633f50c --- /dev/null +++ b/test/table-cell.test.js @@ -0,0 +1,331 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { tableCellTarget } from "../src/table-cell.js"; + +function node(tag, attrs = {}, children = []) { + const el = { + tagName: tag.toUpperCase(), + nodeName: tag.toUpperCase(), + nodeType: 1, + parentElement: null, + children: [], + getAttribute(name) { + return Object.hasOwn(attrs, name) ? String(attrs[name]) : null; + }, + closest(selectorList) { + const tags = selectorList.split(",").map((part) => part.trim()); + let current = el; + while (current) { + if (tags.includes(current.tagName.toLowerCase())) return current; + current = current.parentElement; + } + return null; + }, + }; + if (attrs.textContent) el.textContent = attrs.textContent; + for (const child of children) { + child.parentElement = el; + el.children.push(child); + } + return el; +} + +function row(cells, tag = "td") { + return node( + "tr", + {}, + cells.map((cell) => (typeof cell === "string" ? node(tag, { textContent: cell }) : cell)), + ); +} + +function labels(element) { + const target = tableCellTarget(element); + return { rowLabel: target?.rowLabel, columnLabel: target?.columnLabel }; +} + +test("tableCellTarget names a filtered table cell by row and column instead of visible position", () => { + const target = node("td", { textContent: "Drive, Neovide, Cursor, Alacritty" }); + node("table", {}, [ + node("thead", {}, [row(["Permission / setting", "Visible state", "Database evidence"], "th")]), + node("tbody", {}, [row(["Contacts", "None", "No grants"]), row(["Media & Apple Music", "4 apps", target])]), + ]); + + assert.deepEqual( + tableCellTarget(target, () => "table > tbody > tr:nth-of-type(2) > td:nth-of-type(3)"), + { + type: "table-cell", + selector: "table > tbody > tr:nth-of-type(2) > td:nth-of-type(3)", + rowLabel: "Media & Apple Music", + columnLabel: "Database evidence", + text: "Drive, Neovide, Cursor, Alacritty", + }, + ); +}); + +test("tableCellTarget reads header cells from the first row when the table has no thead", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("tbody", {}, [row(["Permission", "Visible state"], "th"), row(["Media & Apple Music", target])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +// Without a only the first all-th row is the header row, so a second one would otherwise +// fall through to the positional guess and be named after its own sibling column header. +test("tableCellTarget names no row for any all-th header row when the table has no thead", () => { + const target = node("th", { textContent: "Visible state" }); + node("table", {}, [ + node("tbody", {}, [ + node("tr", {}, [node("th", { textContent: "Permission" }), node("th", { colspan: "2", textContent: "State" })]), + node("tr", {}, [node("th", { textContent: "Permission" }), target, node("th", { textContent: "Database" })]), + row(["Media & Apple Music", "4 apps", "Drive"]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +test("tableCellTarget does not treat a leading data row as column headers", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [node("tbody", {}, [row(["Contacts", "None"]), row(["Media & Apple Music", target])])]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "" }); +}); + +test("tableCellTarget stays silent about the column when a grouped header spans it", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [ + node("tr", {}, [node("th", { textContent: "Permission" }), node("th", { colspan: "2", textContent: "State" })]), + ]), + node("tbody", {}, [row(["Media & Apple Music", target, node("td", { textContent: "Drive" })])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "" }); +}); + +test("tableCellTarget names the column from the leaf header row under a grouped header", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [ + node("tr", {}, [node("th", { textContent: "Permission" }), node("th", { colspan: "2", textContent: "State" })]), + row(["Permission", "Visible state", "Database evidence"], "th"), + ]), + node("tbody", {}, [row(["Media & Apple Music", target, node("td", { textContent: "Drive" })])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +// The row's own spans still sum to the header width, so only the rowspan above it reveals that +// this cell renders one column to the right. Positional matching alone would name the column +// "Permission" and the row "4 apps" - the clicked cell's own value passed off as the row's name. +function shiftedByRowSpan(target, rowspan = "2") { + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [ + node("tr", {}, [node("td", { rowspan, textContent: "Media" }), node("td", { textContent: "None" })]), + node("tr", {}, [target, node("td", { textContent: "extra" })]), + ]), + ]); + return target; +} + +test("tableCellTarget names neither coordinate when a rowspan above shifts the row right", () => { + const target = shiftedByRowSpan(node("td", { textContent: "4 apps" })); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +test("tableCellTarget treats rowspan=0 as spanning to the end of the row group", () => { + const target = shiftedByRowSpan(node("td", { textContent: "4 apps" }), "0"); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +test("tableCellTarget restores semantic labels after a finite rowspan ends", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [ + node("tr", {}, [node("td", { rowspan: "2", textContent: "Media" }), node("td", { textContent: "None" })]), + node("tr", {}, [node("td", { textContent: "1 app" })]), + row(["Media & Apple Music", target]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +test("tableCellTarget keeps a declared scope=row heading even when a rowspan shifts the grid", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [ + node("tr", {}, [node("td", { rowspan: "2", textContent: "Media" }), node("td", { textContent: "None" })]), + node("tr", {}, [node("th", { scope: "row", textContent: "Media & Apple Music" }), target]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "" }); +}); + +test("tableCellTarget stays silent about the column when the row does not span the header width", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state", "Database evidence"], "th")]), + node("tbody", {}, [row(["Media & Apple Music", target])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "" }); +}); + +test("tableCellTarget prefers an explicit scope=row heading over the first cell", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [row(["Index", "Permission", "Visible state"], "th")]), + node("tbody", {}, [ + node("tr", {}, [ + node("td", { textContent: "7" }), + node("th", { scope: "row", textContent: "Media & Apple Music" }), + target, + ]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +test("tableCellTarget does not label a header-row click with a data row heading", () => { + const target = node("th", { textContent: "Visible state" }); + node("table", {}, [ + node("thead", {}, [node("tr", {}, [node("th", { textContent: "Permission" }), target])]), + node("tbody", {}, [row(["Media & Apple Music", "4 apps"])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "Visible state" }); +}); + +// The clicked cell sits in the upper row of a grouped header, which `tableHeaderRow` does not +// pick as the header row. Its first sibling is another column header, never this "row" 's name. +test("tableCellTarget does not name an upper grouped-header row after its first cell", () => { + const target = node("th", { colspan: "2", textContent: "State" }); + node("table", {}, [ + node("thead", {}, [ + node("tr", {}, [node("th", { textContent: "Permission" }), target]), + row(["Permission", "Visible state", "Database evidence"], "th"), + ]), + node("tbody", {}, [row(["Media & Apple Music", "4 apps", "Drive"])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +// The grouped-header idiom puts a rowspan in , where it is clipped at the row group +// boundary and cannot reach a body row. Only the leaf header row it shifts loses its names. +test("tableCellTarget still names a body row under a rowspan confined to the header", () => { + const target = node("td", { textContent: "ok" }); + node("table", {}, [ + node("thead", {}, [ + node("tr", {}, [ + node("th", { rowspan: "2", textContent: "Feature" }), + node("th", { textContent: "Result" }), + node("th", { textContent: "Notes" }), + ]), + row(["A", "B", "C"], "th"), + ]), + node("tbody", {}, [row([node("td", { textContent: "Login" }), target, node("td", { textContent: "fine" })])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Login", columnLabel: "" }); +}); + +test("tableCellTarget ignores a rowspan in a row group the clicked row is not in", () => { + const target = node("td", { textContent: "4 apps" }); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [row(["Media & Apple Music", target])]), + node("tfoot", {}, [ + node("tr", {}, [node("td", { rowspan: "2", textContent: "Total" }), node("td", { textContent: "4" })]), + node("tr", {}, [node("td", { textContent: "5" })]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +// HTML's non-negative-integer rules ignore trailing garbage, so this cell really spans two rows. +test("tableCellTarget reads a rowspan attribute the way HTML does, stopping at the first non-digit", () => { + const target = shiftedByRowSpan(node("td", { textContent: "4 apps" }), "2x"); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +test("tableCellTarget trusts the span the browser parsed over the raw attribute", () => { + const target = node("td", { textContent: "4 apps" }); + const spanning = node("td", { rowspan: "junk", textContent: "Media" }); + spanning.rowSpan = 2; + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [ + node("tr", {}, [spanning, node("td", { textContent: "None" })]), + node("tr", {}, [target, node("td", { textContent: "extra" })]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "", columnLabel: "" }); +}); + +test("tableCellTarget reads an empty rowspan attribute the way a browser does, as 1", () => { + const target = node("td", { textContent: "Drive" }); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state", "Database evidence"], "th")]), + node("tbody", {}, [ + node("tr", {}, [ + node("td", { textContent: "Media & Apple Music" }), + node("td", { rowspan: "", textContent: "4 apps" }), + target, + ]), + ]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Database evidence" }); +}); + +test("tableCellTarget bounds every derived string it puts on the wire", () => { + const long = "x".repeat(1000); + const target = node("td", { textContent: long }); + node("table", {}, [node("thead", {}, [row(["Permission", long], "th")]), node("tbody", {}, [row([long, target])])]); + + const result = tableCellTarget(target, () => long); + + assert.equal(result.rowLabel.length, 240); + assert.equal(result.columnLabel.length, 240); + assert.equal(result.text.length, 240); + assert.equal(result.selector.length, 240); +}); + +test("tableCellTarget reads only its own table when a cell holds a nested table", () => { + const target = node("td", { textContent: "4 apps" }); + const nested = node("table", {}, [ + node("tbody", {}, [ + node("tr", {}, [node("td", { rowspan: "2", textContent: "nested span" }), node("td", { textContent: "a" })]), + node("tr", {}, [node("td", { textContent: "b" })]), + ]), + ]); + node("table", {}, [ + node("thead", {}, [row(["Permission", "Visible state"], "th")]), + node("tbody", {}, [row([node("td", { textContent: "Media & Apple Music" }, [nested]), target])]), + ]); + + assert.deepEqual(labels(target), { rowLabel: "Media & Apple Music", columnLabel: "Visible state" }); +}); + +test("tableCellTarget ignores elements outside a table", () => { + const target = node("p", { textContent: "not a cell" }); + node("div", {}, [target]); + + assert.equal(tableCellTarget(target), null); +});