From a484c364293addacd2bf796b60fe67e7f188f000 Mon Sep 17 00:00:00 2001 From: Kyle McDonald Date: Thu, 10 Sep 2026 15:10:10 -0500 Subject: [PATCH] Fix comments on Markdown tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #79. Cards piled onto the top of the table. A Live Preview table is a single block widget, and Obsidian's widget implements no `coordsAt`, so `coordsAtPos` hands back the widget's own rect for every position inside it — every card in a table measured to the same point. Measure the rendered cell instead where the anchor resolves to one; the draft composer takes the same path. An anchor spanning two rows has no single cell to paint, so it reports the cell it starts in and says separately whether it ends there. Commenting a whole row dropped that row and everything below it out of the table; commenting the whole table stopped it rendering as a table at all. Both came from the opening marker landing before a row's leading pipe, and Obsidian keeps an outer-pipe table going only while each line starts with one. A marker on the delimiter row broke it the same way. Anchors are now pulled inside the outer pipes of any row they touch, stepping over the delimiter row and trimming the padding that leaves behind. A selection of nothing but borders is refused with an explanation. The write and the existing-highlight lookup share one normalization, so running Add comment twice on the same text still finds the comment it just made. The clamp covers the edges of a table too. Whitespace is trimmed before deciding which ends sit in a table, so a selection dragged in from the paragraph above starts in the table instead of having its marker dragged onto the header's border, and one that trims to prose stays on the prose. Obsidian starts a table only below a blank line, a heading or the top of the note, so a selection that ends on the blank line above a table (Shift+Down from the start of a paragraph) keeps its markers on the paragraph. Live Preview puts a filled blank line back by itself, but Reading view writes to the file directly, where the table disappeared. A boundary between a backslash and the pipe it escapes widens to keep `\|` whole: a marker between them un-escapes the pipe, splits the cell in two and shows the marker as text. Hovering a card left the text flat and hovering the text never raised its card: inside a table the highlight is painted with the CSS Custom Highlight API, which has no element, and every part of that linkage was looking for a `.doc-comment-span`. The hovered comment is now painted under its own registry name — the same 18% / 38% pair prose uses — and the painted ranges' client rects are hit-tested to find the comment under the pointer. A cell is usually one text node, so mouseover never fires as the pointer crosses into the commented words; mousemove is tracked while over a table instead. That fixes new damage but leaves notes that already have it, where the table stays broken until someone hand-edits an HTML comment — exactly what this plugin exists to avoid. Detection has to be cheap, because the card needs it live, so it runs in two stages: a per-comment string check on the marker's own line (does stripping the markers change whether the line starts with a pipe, ends with one, reads as a delimiter row, or is blank?), read without splitting the document, and only for a line that answers yes, a strip-and-rescan across the blank-line block containing it, since tables never cross one. The margin keeps the result for the document version it came from, so cursor moves and scrolls don't run it again. Code-block comments are skipped; their markers sit on lines of their own by design. Repair reuses the clamp and rebuilds the whole block with every anchor reinserted, not just the repaired ones — a table broken by two anchors only reappears once both are out of the way, and a healthy neighbour's markers have to come back exactly where they were. Only markers the parser recognized are taken out and put back; marker-shaped text inside code, or hidden from the parser by a stray backtick, is left exactly as it was. Markers go back in one forward pass, so an anchor nested inside the one being repaired stays intact, and a comment with no markers in the block, such as an orphan whose text was deleted, is skipped rather than blocking the repair. A comment that cannot be placed is not reported as broken, so a Repair action is never offered that would do nothing. Discovery is the card, which already sits beside the damage: a notice saying the comment is breaking its table, and a one-click Repair. The command repairs the whole note at once, the way out when several anchors broke the same table. Nothing is rewritten unless asked, and both paths are a single undo step. The notice deliberately does not ask the margin to reposition — reconcile runs inside CodeMirror's update cycle and repositioning reads layout, which throws there and takes the whole margin plugin down with it. Underneath all of it, our idea of where a table ends disagreed with Obsidian's. Any line holding a pipe counted as a row. Obsidian continues an outer-pipe table only while a line starts with a pipe, and a pipe-less one only where the tokenizer still sees Markdown — so an unterminated `/; +const ANCHOR_MARKERS = //g; + +/** Every marker-shaped string in `text`. Only the cheap per-line gate uses this. */ +const stripMarkers = (text: string): string => text.replace(ANCHOR_MARKERS, ""); + +/** The anchor markers the parser recognized, in document order. */ +const parsedMarkers = (comments: readonly ParsedComment[]): TextRange[] => { + return comments + .flatMap((comment) => [comment.open, comment.close]) + .filter((marker): marker is TextRange => marker !== null) + .sort((a, b) => a.from - b.from); +}; + +/** + * `text` with just these markers taken out. Marker-shaped text the parser skipped + * — written out inside a code span, say, or hidden by a stray backtick — is + * ordinary text; stripping it too would delete it, because nothing puts it back. + */ +const withoutMarkers = (text: string, markers: readonly TextRange[]): string => { + return ( + markers.map((marker, index) => text.slice(markers[index - 1]?.to ?? 0, marker.from)).join("") + + text.slice(markers[markers.length - 1]?.to ?? 0) + ); +}; + +/** How many marker characters come out before `position`. */ +const strippedBefore = (markers: readonly TextRange[], position: number): number => { + return markers + .filter((marker) => marker.from < position) + .reduce((removed, marker) => removed + (marker.to - marker.from), 0); +}; + +/** The text of the line holding `pos`, read without splitting the document. */ +const lineTextAt = (doc: string, pos: number): string => { + const end = doc.indexOf("\n", pos); + return doc.slice(doc.lastIndexOf("\n", pos - 1) + 1, end < 0 ? doc.length : end); +}; + +/** + * Whether removing the markers from a line changes whether it can take part in + * a table. This is the cheap gate: pure string work on the marker's own line, run + * over every comment on every document change. Nearly every note answers no here + * and nothing in the document is scanned or split. + */ +const markersDamageLine = (text: string): boolean => { + if (!ANCHOR_MARKER.test(text)) return false; + const bare = stripMarkers(text); + // A normal row must START with a pipe, a header must also END with one, a + // delimiter row must be nothing but dashes and colons, and the line above a + // table must be blank for it to render at all. + if ((bare.trim() === "") !== (text.trim() === "")) return true; + if (bare.startsWith("|") !== text.startsWith("|")) return true; + if (/\|\s*$/.test(bare) !== /\|\s*$/.test(text)) return true; + return isDelimiterLine(bare) !== isDelimiterLine(text); +}; + +/** The blank-line-delimited block holding `index`, or null for a line that + * doesn't exist. Tables never cross a blank line. */ +const blockAround = (lines: readonly SourceLine[], index: number): TextRange | null => { + if (!lines[index]) return null; + const blank = lines.map((line) => line.text.trim() === ""); + // A negative fromIndex counts back from the end, so the first line needs a guard. + const gapAbove = index > 0 ? blank.lastIndexOf(true, index - 1) : -1; + const gapBelow = blank.indexOf(true, index + 1); + const first = lines[gapAbove + 1]; + const last = lines[gapBelow < 0 ? lines.length - 1 : gapBelow - 1]; + return first && last ? { from: first.from, to: last.to } : null; +}; + +/** Group comment ids by the block their marker sits in. */ +const groupByBlock = ( + lines: readonly SourceLine[], + markers: ReadonlyArray<{ id: string; at: number }>, +): BlockAnchors[] => { + const groups = markers.reduce((byStart, { id, at }) => { + const block = blockAround(lines, lineIndexAt(lines, at)); + if (!block) return byStart; + const group = byStart.get(block.from) ?? { block, ids: new Set() }; + group.ids.add(id); + return byStart.set(block.from, group); + }, new Map()); + return Array.from(groups.values()); +}; + +/** + * Rewrite one block with `ids`' anchors moved inside their table cells, or null + * when it cannot be done safely. + * + * Every anchor in the block is reinserted, not just the repaired ones: the clamp + * has to run against a block with ALL markers stripped, because a table broken + * by two anchors only reappears once both are out of the way — and a healthy + * neighbour's markers must come back exactly where they were. + */ +const repairedBlock = (doc: string, block: TextRange, ids: ReadonlySet): string | null => { + const text = doc.slice(block.from, block.to); + const comments = parseComments(text); + const found = parsedMarkers(comments); + if (found.length === 0) return null; + const bare = withoutMarkers(text, found); + + // A comment with no markers in the block — an orphan whose anchored text was + // deleted, or a body whose anchor sits elsewhere — has nothing here to move. + const anchored = comments.filter((comment) => comment.open || comment.close); + const placements = anchored.map((comment): Placement | null => { + const range = anchorRange(comment); + // A lone marker has no range to restore, and stripping it would lose it. + if (!range) return null; + const from = range.from - strippedBefore(found, range.from); + const to = range.to - strippedBefore(found, range.to); + const target = ids.has(comment.id) ? clampToTableCells(bare, from, to) : { from, to }; + return target.to > target.from ? { id: comment.id, ...target } : null; + }); + const placed = placements.filter((placement): placement is Placement => placement !== null); + if (placed.length === 0 || placed.length < placements.length) return null; + + // Emit every marker as a point insertion in one forward pass. Splicing whole + // anchors one at a time looks equivalent but silently corrupts nested ranges: + // inserting an inner pair shifts the outer anchor's end, so its close marker + // lands inside the inner one and both comments are destroyed. A whole-table + // anchor around a cell comment — exactly what repair exists to undo — nests. + const markers = placed.flatMap(({ id, from, to }) => [ + { at: from, text: openMarker(id), closing: false, from, to }, + { at: to, text: closeMarker(id), closing: true, from, to }, + ]); + markers.sort((a, b) => { + if (a.at !== b.at) return a.at - b.at; + // At a shared boundary a close comes before an open, so neighbouring anchors + // read `……` rather than interleaving. + if (a.closing !== b.closing) return a.closing ? -1 : 1; + // Innermost closes first and outermost opens first, so nesting stays nested. + return a.closing ? b.from - a.from : b.to - a.to; + }); + + const rebuilt = + markers.map((marker, index) => bare.slice(markers[index - 1]?.at ?? 0, marker.at) + marker.text).join("") + + bare.slice(markers[markers.length - 1]?.at ?? 0); + return rebuilt === text ? null : rebuilt; +}; + +/** + * Ids whose anchor markers are breaking a table AND can be moved back inside it. + * + * Repairability is part of the test on purpose: a comment we cannot put right + * should never be offered a Repair action that does nothing. + */ +export const brokenTableAnchors = (doc: string, parsed?: readonly ParsedComment[]): Set => { + // A code comment's markers sit on lines of their own around a fence by design, + // so a line left blank by stripping them was never a gap a table relied on. + const damaging = (parsed ?? parseComments(doc)) + .filter((comment) => !isCodeComment(comment)) + .flatMap((comment) => + [comment.open, comment.close] + .filter((marker): marker is TextRange => marker !== null) + .filter((marker) => markersDamageLine(lineTextAt(doc, marker.from))) + .map((marker) => ({ id: comment.id, at: marker.from })), + ); + if (damaging.length === 0) return new Set(); + + // Blocks holding a marker that looks damaging, and the ids sitting in them. + const suspects = groupByBlock(sourceLines(doc), damaging); + + // Confirm against the block only — a line can look damaging in prose that was + // never a table. Scoped this way the check stays cheap even when it does run. + const broken = suspects + .filter(({ block }) => { + const text = doc.slice(block.from, block.to); + return tableCoverage(withoutMarkers(text, parsedMarkers(parseComments(text)))) > tableCoverage(text); + }) + .flatMap(({ block, ids }) => Array.from(ids).filter((id) => repairedBlock(doc, block, new Set([id])) !== null)); + return new Set(broken); +}; + +/** Rewrite every block whose table an anchor is breaking. `only` limits it to + * specific comments; without it, every repairable one in the document. */ +export const computeRepairTableAnchors = (doc: string, only?: ReadonlySet): Result => { + const targets = brokenTableAnchors(doc); + const ids = only ? new Set([...targets].filter((id) => only.has(id))) : targets; + if (ids.size === 0) return Result.ok([]); + + const blocks = groupByBlock( + sourceLines(doc), + parseComments(doc).flatMap((comment) => { + const marker = comment.open ?? comment.close; + return ids.has(comment.id) && marker ? [{ id: comment.id, at: marker.from }] : []; + }), + ); + + const changes = blocks.flatMap(({ block, ids: blockIds }) => { + const repaired = repairedBlock(doc, block, blockIds); + return repaired === null ? [] : [{ from: block.from, to: block.to, insert: repaired }]; + }); + return Result.ok(changes.sort((a, b) => a.from - b.from)); +}; diff --git a/src/format/table.ts b/src/format/table.ts new file mode 100644 index 0000000..227b6af --- /dev/null +++ b/src/format/table.ts @@ -0,0 +1,295 @@ +import { TextRange } from "./types"; + +export type SourceLine = { text: string; from: number; to: number }; +/** A table's line span (`start` inclusive, `end` exclusive) and offset span. */ +export type SourceTable = { start: number; end: number; from: number; to: number }; +type TableKind = "normal" | "simple"; + +// Obsidian decides where a table starts and stops with these line shapes, and +// the Live-Preview table widget spans exactly the run of lines they cover. Ours +// has to agree: a looser rule hands the renderer a row index its DOM doesn't +// have, and a stricter one drops a table the reader can plainly see. +// +// A "normal" table draws outer pipes and continues only while a line STARTS with +// one — which is why a row that doesn't (one whose leading pipe a comment marker +// displaced, say) truncates the table there. A "simple" table draws no outer +// pipes and continues while a line starts with a non-pipe and contains a pipe. +// +// These are Obsidian's own patterns, lifted from its bundled tokenizer, and they +// are deliberately stricter than GFM. Each of these GFM-valid shapes renders as +// plain text in Obsidian, with no table widget — checked in the app: +// +// indented by spaces or a tab · nested in a list item · inside a blockquote · +// a header with only one of its two outer pipes +// +// Don't widen the patterns to catch them. With no widget there is nothing to +// measure a row against, so the ordinary text path already highlights and aligns +// their comments, and a marker can't break a table that never rendered as one. +// test/table-anchors.test.ts pins each shape. +const NORMAL_HEADER = /^\|(?:[^|]+\|)+?\s*$/; +const SIMPLE_HEADER = /^\s*[^|].*?\|.*[^|]\s*$/; +const NORMAL_ROW = /^\|/; +const SIMPLE_ROW = /^\s*[^|].*\|/; +// One dash per column is enough here — Obsidian is looser than GFM's three. +const DELIMITER_CELL = /^\s*:?\s*-+\s*:?\s*$/; +// Setext underlines are let through with ATX headings. Treating one as a table's +// opener errs toward recognizing a table Obsidian doesn't draw, which is harmless; +// the other way round would strip a real table of its clamp and its alignment. +const HEADING = /^ {0,3}(?:#{1,6}(?:\s|$)|=+\s*$|-+\s*$)/; + +export const sourceLines = (doc: string): SourceLine[] => { + const lines: SourceLine[] = []; + let from = 0; + for (const text of doc.split("\n")) { + lines.push({ text, from, to: from + text.length }); + from += text.length + 1; + } + return lines; +}; + +export const sourceTables = (lines: readonly SourceLine[]): SourceTable[] => { + // Scanner that consumes a variable run of rows per table and advances `start` + // past it — a for loop is the natural fit, not an array method. + const tables: SourceTable[] = []; + for (let start = 0; start + 1 < lines.length; start++) { + const head = lines[start]; + const delimiter = lines[start + 1]; + if (!head || !delimiter || !opensTableBelow(lines[start - 1])) continue; + const kind = tableKind(head.text, delimiter.text); + if (!kind) continue; + const isRow = + kind === "normal" + ? (text: string) => NORMAL_ROW.test(text) + : (text: string) => SIMPLE_ROW.test(tokenizable(text)); + let end = start + 2; + let row = lines[end]; + while (row && isRow(row.text)) { + end++; + row = lines[end]; + } + const lastRow = lines[end - 1]; + if (!lastRow) continue; + tables.push({ start, end, from: head.from, to: lastRow.to }); + start = end - 1; + } + return tables; +}; + +/** + * The part of a line still tokenized as Markdown. Text after an HTML comment + * that does NOT close on the line sits inside a comment token running past the + * line end, so a pipe there never becomes a cell separator. + * + * That is what lets a comment's own `` block sit directly beneath a + * pipe-less table without being read as another row, even when its `quote:` + * carries a pipe — confirmed in the app, where the same table followed by a + * plain `ordinary line | with a pipe` DOES grow a row. A comment that opens and + * closes on the line (an anchor marker) leaves the rest of the line tokenizing + * normally, so a row starting with one is still a row. + */ +const tokenizable = (line: string): string => { + // A scan, not a transform: each step depends on where the previous comment + // closed, and it exits at whichever of two conditions comes first. + let cursor = 0; + for (;;) { + const open = line.indexOf("", open + 4); + if (close < 0) return line.slice(0, open); + cursor = close + 3; + } +}; + +/** + * Whether a table may start on the line after `above`. Obsidian's tokenizer only + * looks for a header below a blank line, a heading, or the top of the note + * (`prevLine.stream.string.trim() && !wasHeading`). Checked in the app: prose, a + * comment's own block, or a marker alone on the line above leaves the whole table + * as plain text. + */ +const opensTableBelow = (above: SourceLine | undefined): boolean => { + return !above || above.text.trim() === "" || HEADING.test(above.text); +}; + +/** Which flavor of table `head` opens, or null when it opens none. */ +const tableKind = (head: string, delimiter: string): TableKind | null => { + if (NORMAL_HEADER.test(head)) { + if (!NORMAL_HEADER.test(delimiter)) return null; + // The outer pipes aren't columns, so drop them before splitting. + return isDelimiterRow(delimiter.replace(/^\s*\|/, "").replace(/\|\s*$/, "")) ? "normal" : null; + } + if (SIMPLE_HEADER.test(head)) { + if (!SIMPLE_HEADER.test(delimiter)) return null; + return isDelimiterRow(delimiter) ? "simple" : null; + } + return null; +}; + +const isDelimiterRow = (line: string): boolean => { + return line.split("|").every((cell) => DELIMITER_CELL.test(cell)); +}; + +/** Whether a whole source line reads as a table's delimiter row, either flavor. */ +export const isDelimiterLine = (line: string): boolean => { + const inner = line.replace(/^\s*\|/, "").replace(/\|\s*$/, ""); + return inner.trim().length > 0 && isDelimiterRow(inner); +}; + +/** Total rows every table in `text` covers. Rises when a repair reveals more table. */ +export const tableCoverage = (text: string): number => { + return sourceTables(sourceLines(text)).reduce((rows, table) => rows + (table.end - table.start), 0); +}; + +/** Zero-based column of `offset` within a table row, ignoring the leading pipe. */ +export const tableColumnAt = (line: string, offset: number): number => { + const pipes = unescapedPipes(line); + const firstNonSpace = line.search(/\S/); + const leadingPipe = pipes[0] === firstNonSpace ? pipes[0] : null; + return pipes.filter((pipe) => pipe < offset && pipe !== leadingPipe).length; +}; + +const unescapedPipes = (line: string): number[] => { + const pipes: number[] = []; + for (let i = 0; i < line.length; i++) { + if (line[i] !== "|") continue; + let slashes = 0; + for (let j = i - 1; j >= 0 && line[j] === "\\"; j--) slashes++; + if (slashes % 2 === 0) pipes.push(i); + } + return pipes; +}; + +/** + * Pull a selection inside the outer pipes of any table row it touches, then trim + * the whitespace that padding leaves on the edges. + * + * A comment marker written outside those pipes breaks the row it lands on: + * Obsidian keeps an outer-pipe table going only while each line starts with a + * pipe, and reads a header only while it ends with one. So anchoring a whole row + * used to drop that row and everything after it out of the rendered table, and + * anchoring a whole table stopped it rendering as a table at all. Inside the + * pipes the markers are ordinary cell text and the table holds together. + * + * Returns an empty range when the selection held nothing but borders and + * padding, which the caller reports rather than anchoring. + */ +export const clampToTableCells = (doc: string, from: number, to: number): TextRange => { + if (to <= from) return { from, to }; + const lines = sourceLines(doc); + const tables = sourceTables(lines); + if (tables.length === 0) return { from, to }; + const touchesTable = !!tableAt(tables, lineIndexAt(lines, from)) || !!tableAt(tables, lineIndexAt(lines, to)); + + // Trim BEFORE deciding which ends sit in a table. Trimmed afterwards, an end + // left outside the table gets dragged across blank lines onto the table's own + // border, and a marker there stops it rendering. Trimmed first, a selection + // starting on the blank line above a table starts in the table and is clamped + // like one, while one starting in prose stays on that prose. + const selected = trimRange(doc, from, to); + const fromIndex = lineIndexAt(lines, selected.from); + const toIndex = lineIndexAt(lines, selected.to); + const head = tableAt(tables, fromIndex); + const tail = tableAt(tables, toIndex); + if (!head && !tail) { + // Only whitespace tied the selection to a table, or an end sits on the blank + // line a table needs above it — as when a triple-click takes a paragraph's + // newline. Either way the markers belong on the selected text. A selection + // that touches neither is left exactly as it was. + const fillsGap = [from, to].some((pos) => isGapAboveTable(lines, tables, lineIndexAt(lines, pos))); + return touchesTable || fillsGap ? selected : { from, to }; + } + + const start = head ? anchorablePosition(lines, head, fromIndex, selected.from, 1) : selected.from; + const end = tail ? anchorablePosition(lines, tail, toIndex, selected.to, -1) : selected.to; + if (start === null || end === null || end <= start) return { from, to: from }; + const trimmed = trimRange(doc, start, end); + if (trimmed.to <= trimmed.from) return { from, to: from }; + // A marker between a backslash and the pipe it escapes un-escapes that pipe, + // splitting the cell in two and showing the marker as text. Keep the pair whole. + const range = { + from: splitsEscapedPipe(doc, trimmed.from) ? trimmed.from - 1 : trimmed.from, + to: splitsEscapedPipe(doc, trimmed.to) ? trimmed.to + 1 : trimmed.to, + }; + // Clamping only moves the two ends, so a selection between two rows or around a + // separator still holds nothing but the pipes that divide cells. + return holdsCellText(doc, range) ? range : { from, to: from }; +}; + +/** Whether `pos` falls between a backslash and the pipe it escapes. */ +const splitsEscapedPipe = (doc: string, pos: number): boolean => { + if (doc[pos] !== "|") return false; + const before = doc.slice(doc.lastIndexOf("\n", pos - 1) + 1, pos); + const slashes = before.length - before.replace(/\\+$/, "").length; + return slashes % 2 === 1; +}; + +/** Whether `range` holds anything besides whitespace and unescaped cell pipes. */ +const holdsCellText = (doc: string, range: TextRange): boolean => { + // Scan from the line start: the backslash escaping a pipe can sit just before + // the range, and a pipe it escapes is cell text, not a border. + const lineStart = doc.lastIndexOf("\n", range.from - 1) + 1; + const pipes = new Set(unescapedPipes(doc.slice(lineStart, range.to)).map((pipe) => lineStart + pipe)); + return doc + .slice(range.from, range.to) + .split("") + .some((char, index) => !pipes.has(range.from + index) && char.trim() !== ""); +}; + +/** Whether a line is the blank one a table depends on to render at all. */ +const isGapAboveTable = (lines: readonly SourceLine[], tables: readonly SourceTable[], lineIndex: number): boolean => { + return lines[lineIndex]?.text.trim() === "" && tables.some((table) => table.start === lineIndex + 1); +}; + +const tableAt = (tables: readonly SourceTable[], lineIndex: number): SourceTable | undefined => { + return tables.find((table) => lineIndex >= table.start && lineIndex < table.end); +}; + +/** + * Where a marker may sit at or beyond `pos`, searching in `step`'s direction for + * the first row of `table` that can hold one. Null when the table has no such row. + */ +const anchorablePosition = ( + lines: readonly SourceLine[], + table: SourceTable, + lineIndex: number, + pos: number, + step: 1 | -1, +): number | null => { + // Walks in a caller-chosen direction and stops at the first usable row, which + // no single array method expresses. + for (let index = lineIndex; index >= table.start && index < table.end; index += step) { + // The delimiter row has to stay nothing but dashes and colons, so a marker + // dropped there stops the whole block reading as a table. Step over it. + if (index === table.start + 1) continue; + const line = lines[index]; + if (!line) continue; + const bounds = cellBounds(line); + if (bounds.to <= bounds.from) continue; // borders all the way across + return Math.min(Math.max(pos, bounds.from), bounds.to); + } + return null; +}; + +/** The offsets a table row's cell content may occupy, excluding its outer pipes. */ +const cellBounds = (line: SourceLine): TextRange => { + const pipes = unescapedPipes(line.text); + const first = pipes[0]; + const last = pipes[pipes.length - 1]; + const start = first !== undefined && first === line.text.search(/\S/) ? first + 1 : 0; + // Only a pipe with nothing but whitespace after it is a closing border; on a + // row without one the last pipe is a cell separator and content runs past it. + const end = last !== undefined && line.text.slice(last + 1).trim() === "" ? last : line.text.length; + return { from: line.from + start, to: line.from + Math.max(start, end) }; +}; + +export const lineIndexAt = (lines: readonly SourceLine[], pos: number): number => { + return lines.findIndex((line) => pos >= line.from && pos <= line.to); +}; + +const trimRange = (doc: string, from: number, to: number): TextRange => { + const text = doc.slice(from, to); + const leading = text.length - text.trimStart().length; + const trailing = text.length - text.trimEnd().length; + const start = from + leading; + return { from: start, to: Math.max(start, to - trailing) }; +}; diff --git a/src/main.ts b/src/main.ts index a727706..c78cc55 100644 --- a/src/main.ts +++ b/src/main.ts @@ -18,7 +18,7 @@ import { marginPlugin } from "./editor/margin"; import { commentConfig } from "./editor/config"; import { editorLayoutField } from "./editor/layout"; import { draftField, setDraft } from "./editor/draft"; -import { addComment, insertCommentInFile } from "./editor/commands"; +import { addComment, insertCommentInFile, repairTableAnchors } from "./editor/commands"; import { findHighlightAtSelection } from "./editor/edits"; import { findSectionRange, highlightPostProcessor, mapReadingSelection } from "./reading/highlight"; import { ReadingDeps, ReadingMarginManager } from "./reading/margin"; @@ -26,6 +26,7 @@ import { COMMENTS_VIEW_TYPE, CommentsSidebarView, SidebarDeps } from "./ui/sideb import { CommentModal } from "./ui/comment-modal"; import { DEFAULT_SETTINGS, DocCommentsSettings, DocCommentsSettingTab } from "./settings"; import { tableHighlightPlugin } from "./editor/table-highlights"; +import { brokenTableAnchors } from "./editor/table-repair"; import { authorColorCss, canonicalAuthorKey, @@ -191,6 +192,12 @@ export default class DocCommentsPlugin extends Plugin { editorCallback: (editor) => this.startAddComment(editor), }); + this.addCommand({ + id: "repair-table-comments", + name: "Repair table comments in this note", + editorCallback: (editor) => this.repairTableComments(editor), + }); + this.addCommand({ id: "toggle-comments", name: "Toggle comments", @@ -232,6 +239,28 @@ export default class DocCommentsPlugin extends Plugin { this.addSettingTab(this.settingsTab); } + /** Move every anchor that is breaking a table in this note back inside its + * cell. The card offers the same repair one comment at a time; this is the + * way out when several anchors broke the same table. */ + private repairTableComments(editor: Editor): void { + const view = editorView(editor); + if (!view) { + new Notice("Couldn't access the editor."); + return; + } + const broken = brokenTableAnchors(view.state.doc.toString()); + if (broken.size === 0) { + new Notice("No table comments need repairing."); + return; + } + const result = repairTableAnchors(view); + if (result.isErr()) { + new Notice(`Couldn't repair the table comments: ${result.error}`); + return; + } + new Notice(`Repaired ${broken.size} table ${broken.size === 1 ? "comment" : "comments"}.`); + } + private startAddComment(editor: Editor): void { const view = editorView(editor); if (!view) { diff --git a/src/ui/card.ts b/src/ui/card.ts index bdc64b6..a146abc 100644 --- a/src/ui/card.ts +++ b/src/ui/card.ts @@ -32,6 +32,9 @@ export type CardCallbacks = { /** Reveal this thread in the comments sidebar — the escape for a card too tall to * fit the margin even when expanded. Absent for cards already in the sidebar. */ openInSidebar?: (id: string) => void; + /** Move this comment's anchor back inside its table cell. Absent where repair + * isn't offered, in which case the card shows no repair notice at all. */ + repairTableAnchor?: (id: string) => void; }; /** Per-view context a card needs to render comment text as Markdown. */ @@ -63,6 +66,8 @@ export class Card { private overflows = false; private tooTall = false; private clipEl: HTMLElement | null = null; + /** This comment's anchor is breaking the table it sits in (see table-repair). */ + private tableAnchorBroken = false; private threadEl: HTMLElement | null = null; private footEl: HTMLElement | null = null; /** Owns the child components MarkdownRenderer attaches (link/embed handlers). */ @@ -100,6 +105,35 @@ export class Card { return this.comment.id; } + /** Show or hide the "this comment is breaking its table" notice. Cheap enough + * to call on every reconcile; only touches the DOM when the state flips. + * + * Deliberately does NOT call onResize: reconcile runs inside CodeMirror's + * update cycle, and repositioning reads layout, which throws there. The + * margin already schedules a measure pass after every reconcile, so the + * height change this causes is picked up then. */ + setTableAnchorBroken(broken: boolean): void { + if (this.tableAnchorBroken === broken) return; + this.tableAnchorBroken = broken; + this.renderRepairNotice(); + } + + /** A card whose anchor broke its table sits beside the damage, so this is where + * the reader is already looking when they wonder what went wrong. */ + private renderRepairNotice(): void { + this.el.querySelector(".dc-repair")?.remove(); + if (!this.tableAnchorBroken || !this.cb.repairTableAnchor) return; + const notice = createDiv("dc-repair"); + setIcon(notice.createSpan("dc-repair__icon"), "alert-triangle"); + notice.createSpan({ cls: "dc-repair__text", text: "This comment is breaking its table." }); + const button = notice.createEl("button", { cls: "dc-repair__action", text: "Repair" }); + button.addEventListener("click", (e) => { + e.stopPropagation(); + this.cb.repairTableAnchor?.(this.id); + }); + this.el.prepend(notice); + } + get signature(): string { return cardSignature(this.comment); } @@ -190,6 +224,7 @@ export class Card { // The thread lives in a clip wrapper that gets a max-height when a tall card is // collapsed; the footer (Show more / Open in sidebar) sits outside the clip. + this.renderRepairNotice(); const clip = this.el.createDiv("dc-card-clip"); this.clipEl = clip; const thread = clip.createDiv("dc-thread"); diff --git a/styles.css b/styles.css index 4b5ef25..d976879 100644 --- a/styles.css +++ b/styles.css @@ -623,3 +623,50 @@ body { min-height: 5em; resize: vertical; } + +/* ── Broken table anchor ────────────────────────────────────────────────── */ +/* A comment written before the anchor clamp can sit outside its row's outer + pipes and stop the block rendering as a table. The card is already anchored + beside the damage, so it is where the reader is looking when they wonder what + went wrong — hence a notice on the card rather than a command they'd have to + know to go find. */ +.dc-repair { + display: flex; + align-items: center; + gap: 8px; + margin: calc(-1 * var(--dc-card-pad-y, 10px)) -12px 8px -12px; + padding: 7px 10px; + border-bottom: 1px solid var(--background-modifier-border); + border-radius: var(--radius-m, 8px) var(--radius-m, 8px) 0 0; + background-color: color-mix(in srgb, var(--text-warning) 10%, transparent); + font-size: var(--font-ui-smaller); +} +.dc-repair__icon { + display: flex; + flex: 0 0 auto; + color: var(--text-warning); +} +.dc-repair__icon svg { + width: 14px; + height: 14px; +} +.dc-repair__text { + flex: 1 1 auto; + color: var(--text-muted); + line-height: 1.35; +} +.dc-repair__action { + flex: 0 0 auto; + padding: 2px 6px; + border: none; + border-radius: 4px; + background: none; + box-shadow: none; + color: var(--text-accent); + font-size: var(--font-ui-smaller); + font-weight: var(--font-medium); + cursor: pointer; +} +.dc-repair__action:hover { + background-color: var(--background-modifier-hover); +} diff --git a/test/card.test.ts b/test/card.test.ts index bd92b96..263389a 100644 --- a/test/card.test.ts +++ b/test/card.test.ts @@ -84,6 +84,48 @@ const callbacks = (): CardCallbacks => ({ toggleReaction: vi.fn(), }); +describe("broken table anchor notice", () => { + const view = { sourcePath: () => "note.md", colorForAuthor: () => null }; + + test("shows the notice and repair action only while the anchor is breaking a table", () => { + const repairTableAnchor = vi.fn(); + const card = new Card(commentWithText(), { ...callbacks(), repairTableAnchor }, view); + + expect(card.el.querySelector(".dc-repair")).toBeNull(); + + card.setTableAnchorBroken(true); + expect(card.el.querySelector(".dc-repair__text")?.textContent).toBe("This comment is breaking its table."); + card.el.querySelector(".dc-repair__action")?.click(); + expect(repairTableAnchor).toHaveBeenCalledWith(commentWithText().id); + + card.setTableAnchorBroken(false); + expect(card.el.querySelector(".dc-repair")).toBeNull(); + }); + + test("stays silent when no repair action is available, as in the sidebar", () => { + const card = new Card(commentWithText(), callbacks(), view); + + card.setTableAnchorBroken(true); + + expect(card.el.querySelector(".dc-repair")).toBeNull(); + }); + + test("never asks the margin to reposition", () => { + // The margin reconciles inside CodeMirror's update cycle, and repositioning + // reads layout — which throws there, taking the whole margin plugin down with + // it. The scheduled measure pass after each reconcile picks the height change + // up instead. + const cb = { ...callbacks(), repairTableAnchor: vi.fn() }; + const card = new Card(commentWithText(), cb, view); + (cb.onResize as ReturnType).mockClear(); + + card.setTableAnchorBroken(true); + card.setTableAnchorBroken(false); + + expect(cb.onResize).not.toHaveBeenCalled(); + }); +}); + describe("empty comment card", () => { test("colors every displayed author name with that author's assignment", () => { const comment = { diff --git a/test/table-anchors.test.ts b/test/table-anchors.test.ts new file mode 100644 index 0000000..2655dc1 --- /dev/null +++ b/test/table-anchors.test.ts @@ -0,0 +1,287 @@ +import { describe, expect, test } from "vitest"; +import { applyChanges, computeAddComment } from "../src/editor/edits"; +import { clampToTableCells, sourceLines, sourceTables } from "../src/format/table"; + +// Obsidian keeps an outer-pipe table going only while a line STARTS with a pipe, +// and a pipe-less one while a line starts with a non-pipe and holds a pipe. A +// header is read only while it also ENDS with a pipe, and a delimiter row only +// while every cell is dashes and colons. Mirroring those rules here is what makes +// "the table still renders" an assertion rather than a hope. +// +// One rule isn't a line shape. A pipe only separates cells where the tokenizer +// still sees Markdown, so text after an HTML comment that does NOT close on the +// line contributes nothing. All three cases were checked in the app: +// +// pipe-less table + `ordinary line | with a pipe` -> grows a row +// pipe-less table + `Monday | spec` -> still a row +// +// So a comment's own block can sit directly beneath a table, and a marker at the +// start of a pipe-less row is harmless. An outer-pipe row is different: its +// `^\|` test reads the raw line, so a marker before the leading pipe breaks it. +// +// Nor does a table start just anywhere: the tokenizer only looks for a header +// below a blank line, a heading, or the top of the note. Checked in the app, prose +// or a marker alone on the line above leaves the whole table as plain text. +const NORMAL_HEADER = /^\|(?:[^|]+\|)+?\s*$/; +const SIMPLE_HEADER = /^\s*[^|].*?\|.*[^|]\s*$/; +const DELIMITER_CELL = /^\s*:?\s*-+\s*:?\s*$/; +const HEADING = /^ {0,3}#{1,6}(?:\s|$)/; +/** The part of a line still tokenized as Markdown (see above). */ +const tokenizable = (line: string): string => { + // A scan, not a transform: each step depends on where the previous comment + // closed, and it exits at whichever of two conditions comes first. + let cursor = 0; + for (;;) { + const open = line.indexOf("", open + 4); + if (close < 0) return line.slice(0, open); + cursor = close + 3; + } +}; + +const renderedRows = (doc: string): number | null => { + const lines = doc.split("\n"); + // Finds the first table, then consumes its variable run of rows from there — + // two linked scans with an early exit, which no array method chain expresses. + for (let i = 0; i + 1 < lines.length; i++) { + const head = lines[i] ?? ""; + const delimiter = lines[i + 1] ?? ""; + const above = lines[i - 1]; + if (above !== undefined && above.trim() !== "" && !HEADING.test(above)) continue; + const normal = NORMAL_HEADER.test(head) && NORMAL_HEADER.test(delimiter); + const simple = !normal && SIMPLE_HEADER.test(head) && SIMPLE_HEADER.test(delimiter); + if (!normal && !simple) continue; + const cells = normal ? delimiter.replace(/^\s*\|/, "").replace(/\|\s*$/, "") : delimiter; + if (!cells.split("|").every((cell) => DELIMITER_CELL.test(cell))) continue; + const isRow = (text: string) => (normal ? text.startsWith("|") : /^\s*[^|].*\|/.test(tokenizable(text))); + let end = i + 2; + while (end < lines.length && isRow(lines[end] ?? "")) end++; + return end - i - 1; + } + return null; +}; + +const addComment = (doc: string, from: number, to: number): string | null => { + const result = computeAddComment(doc, from, to, { + id: "aa11", + createdAt: "2026-01-01T00:00:00.000Z", + author: "me", + text: "hi", + }); + return result.isErr() ? null : applyChanges(doc, result.value); +}; + +const normal = [ + "| Day | Task | Owner |", + "| --- | --- | --- |", + "| Monday | write the spec | ana |", + "| Tuesday | review the spec | ben |", +].join("\n"); +const simple = ["Day | Task", "--- | ---", "Monday | write the spec", "Tuesday | review the spec"].join("\n"); + +const span = (doc: string, text: string): [number, number] => { + const from = doc.indexOf(text); + expect(from, `missing ${text}`).toBeGreaterThanOrEqual(0); + return [from, from + text.length]; +}; + +/** The span of `part` where it first appears inside `context`. */ +const spanWithin = (doc: string, context: string, part: string): [number, number] => { + const [start] = span(doc, context); + const from = start + context.indexOf(part); + return [from, from + part.length]; +}; + +describe("anchoring inside a table", () => { + test.each([ + ["one cell", ...span(normal, "write the spec")], + ["a whole row", ...span(normal, "| Monday | write the spec | ana |")], + ["the header row", ...span(normal, "| Day | Task | Owner |")], + ["two cells of one row", ...span(normal, "Monday | write the spec")], + ["two rows", ...span(normal, "write the spec | ana |\n| Tuesday | review")], + ["the whole table", 0, normal.length], + ["the header and delimiter", 0, normal.indexOf("| Monday") - 1], + ])("keeps an outer-pipe table rendering when anchoring %s", (_label, from, to) => { + const out = addComment(normal, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(normal)); + }); + + // Only an end that lands in a table gets pulled inside its pipes. The other end + // stays where the selection put it, so it can't be dragged onto a border. + const between = ["Intro text", "", normal, "", "After."].join("\n"); + const headerOnly = ["Intro text", "", "| Day | Task |", "| --- | --- |", "", "After."].join("\n"); + test.each([ + ["starts at the end of the paragraph above", between, between.indexOf("\n\n"), between.indexOf("ana") + 3], + ["starts on the blank line above", between, between.indexOf("\n\n") + 1, between.indexOf("ana") + 3], + ["ends on the blank line above, as a triple-click does", between, 0, between.indexOf("\n\n") + 1], + [ + "ends on the blank line below a header-only table", + headerOnly, + headerOnly.indexOf("Day"), + headerOnly.lastIndexOf("\n\n") + 1, + ], + ["ends at the start of the paragraph below", between, between.indexOf("write"), between.indexOf("After.")], + [ + "starts after a table's last pipe and runs into the paragraph below", + headerOnly, + headerOnly.indexOf("| --- | --- |") + "| --- | --- |".length, + headerOnly.length, + ], + ])("keeps a table rendering when a selection %s", (_label, doc, from, to) => { + const out = addComment(doc, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(doc)); + }); + + test.each([ + ["one cell", ...span(simple, "write the spec")], + ["a whole row", ...span(simple, "Monday | write the spec")], + ["the whole table", 0, simple.length], + ])("keeps a pipe-less table rendering when anchoring %s", (_label, from, to) => { + const out = addComment(simple, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(simple)); + }); + + test("anchors a whole row inside its outer pipes", () => { + const out = addComment(normal, ...span(normal, "| Monday | write the spec | ana |")); + + expect(out).toContain("| Monday | write the spec | ana |"); + }); + + test.each([ + ["where the header meets the delimiter", normal, ...span(normal, "|\n|")], + ["between two body rows", normal, ...spanWithin(normal, "ana |\n| Tuesday", "|\n|")], + ["around one cell separator", normal, ...spanWithin(normal, "Monday | write", " | ")], + ["around a pipe-less cell separator", simple, ...spanWithin(simple, "Monday | write", " | ")], + ])("refuses a selection of nothing but borders %s", (_label, doc, from, to) => { + const result = computeAddComment(doc, from, to, { + id: "aa11", + createdAt: "2026-01-01T00:00:00.000Z", + author: "me", + text: "hi", + }); + + expect(result.isErr() && result.error).toBe("Select the text inside a table cell, not its borders."); + }); + + // A marker between a backslash and the pipe it escapes un-escapes that pipe: in + // the app the cell splits in two and the marker itself shows as text. + test.each([ + ["the escaped pipe", "\\|", "\\|"], + ["just its pipe", "|", "\\|"], + ["text ending on its backslash", "this \\", "this \\|"], + ["text starting on its pipe", "| that", "\\| that"], + ])("keeps an escaped pipe whole when anchoring %s", (_label, part, anchored) => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\| that |"].join("\n"); + const [from, to] = spanWithin(doc, "this \\| that", part); + const [anchoredFrom, anchoredTo] = spanWithin(doc, "this \\| that", anchored); + + expect(clampToTableCells(doc, from, to)).toEqual({ from: anchoredFrom, to: anchoredTo }); + }); + + test("treats a pipe after an escaped backslash as the separator it is", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\\\| that |"].join("\n"); + const [from, to] = spanWithin(doc, "this \\\\| that", "this \\\\"); + + expect(clampToTableCells(doc, from, to)).toEqual({ from, to }); + }); + + test("writes the markers outside an escaped pipe, never between it and its backslash", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\| that |"].join("\n"); + + expect(addComment(doc, ...spanWithin(doc, "this \\| that", "|"))).toContain( + "| Monday | this \\| that |", + ); + }); + + test("leaves a collapsed selection alone", () => { + const at = normal.indexOf("Monday"); + + expect(clampToTableCells(normal, at, at)).toEqual({ from: at, to: at }); + }); + + test("pulls a selection starting above a table into its first cell", () => { + const out = addComment(between, between.indexOf("\n\n"), between.indexOf("ana") + 3); + + expect(out).toContain("Intro text\n\n| Day | Task | Owner |"); + }); + + test("keeps a triple-clicked paragraph's markers off the blank line above a table", () => { + const out = addComment(between, 0, between.indexOf("\n\n") + 1); + + expect(out).toContain("Intro text\n"); + expect(out).toContain("-->\n\n| Day | Task | Owner |"); + }); + + test("leaves a prose selection and its trailing whitespace exactly as they were", () => { + // The blank line this one ends on sits above more prose, not a table. + const doc = ["Intro text", "", "More prose.", "", normal].join("\n"); + const to = doc.indexOf("\n\n") + 1; + + expect(clampToTableCells(doc, 0, to)).toEqual({ from: 0, to }); + }); + + test("leaves a selection outside any table alone", () => { + const doc = "Some prose with a | pipe in it."; + + expect(clampToTableCells(doc, 5, 10)).toEqual({ from: 5, to: 10 }); + }); + + test("trims the border and padding a clamped selection picks up", () => { + const [from, to] = span(normal, "| Monday "); + + expect(clampToTableCells(normal, from, to)).toEqual({ + from: normal.indexOf("Monday"), + to: normal.indexOf("Monday") + "Monday".length, + }); + }); + + test("keeps an interior separator, which is a real multi-cell selection", () => { + const [from, to] = span(normal, "Monday | write"); + + expect(clampToTableCells(normal, from, to)).toEqual({ from, to }); + }); +}); + +// Every shape below is valid GFM, and every one renders as plain text in Obsidian +// — checked in the app, where only the control mounted a `.cm-table-widget`. A +// scanner that matched them would hand the margin row indices no widget has. +describe("recognizing only the tables Obsidian renders", () => { + const tableCount = (doc: string) => sourceTables(sourceLines(doc)).length; + + test("finds a table with both outer pipes", () => { + expect(tableCount("| Day | Task |\n| --- | --- |\n| Monday | spec |")).toBe(1); + }); + + test.each([ + ["indented by spaces", " | Day | Task |\n | --- | --- |\n | Monday | spec |"], + ["indented by a tab", "\t| Day | Task |\n\t| --- | --- |\n\t| Monday | spec |"], + ["nested in a list item", "- plan\n | Day | Task |\n | --- | --- |\n | Monday | spec |"], + ["inside a blockquote", "> | Day | Task |\n> | --- | --- |\n> | Monday | spec |"], + ["headed with only a leading pipe", "| Day | Task\n| --- | ---\n| Monday | spec"], + ["headed with only a trailing pipe", "Day | Task |\n--- | --- |\nMonday | spec |"], + ["directly below a line of prose", "Intro text\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + [ + "directly below a marker alone on its line", + "\n| Day | Task |\n| --- | --- |\n| Monday | spec |", + ], + ["directly below a comment's own block", '\n| Day | Task |\n| --- | --- |'], + ])("finds no table %s", (_shape, doc) => { + expect(tableCount(doc)).toBe(0); + }); + + test.each([ + ["below a blank line", "Intro text\n\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ["below a line of only spaces", "Intro text\n \n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ["directly below a heading", "## Plan\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ])("finds a table %s", (_shape, doc) => { + expect(tableCount(doc)).toBe(1); + }); +}); diff --git a/test/table-highlights.test.ts b/test/table-highlights.test.ts index dd72661..07ec7ea 100644 --- a/test/table-highlights.test.ts +++ b/test/table-highlights.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "vitest"; import { parseComments } from "../src/format/parse"; import { mapTableWidgets, + tableCellForRange, tableHighlightName, tableHighlightRule, tableHighlightTargets, @@ -22,8 +23,8 @@ describe("tableHighlightTargets", () => { ].join("\n"); expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ - { table: 0, row: 0, column: 0, quote: "Day", resolved: true, author: "me" }, - { table: 0, row: 1, column: 1, quote: "ship", resolved: false, author: "me" }, + { table: 0, row: 0, column: 0, id: "h1", quote: "Day", resolved: true, author: "me" }, + { table: 0, row: 1, column: 1, id: "t1", quote: "ship", resolved: false, author: "me" }, ]); }); @@ -45,11 +46,96 @@ describe("tableHighlightTargets", () => { ].join("\n"); expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ - { table: 0, row: 1, column: 1, quote: "two", resolved: false, author: "me" }, - { table: 1, row: 1, column: 0, quote: "three", resolved: false, author: "me" }, + { table: 0, row: 1, column: 1, id: "a1", quote: "two", resolved: false, author: "me" }, + { table: 1, row: 1, column: 0, id: "b1", quote: "three", resolved: false, author: "me" }, ]); }); + test("stops a pipe table where Obsidian stops it, not at the last line with a pipe", () => { + // Obsidian continues an outer-pipe table only while a line STARTS with a pipe. + // Treating any line containing one as a row used to claim the prose below as + // row 2 and hunt for a cell the rendered table doesn't have. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "Prose with a | pipe and a commented phrase here.", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test("stops at a row whose leading pipe a comment marker displaced", () => { + // Anchoring a whole row puts the opening marker before the leading pipe, which + // drops that row out of the table — so no cell exists to highlight. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test("accepts the single-dash delimiter row Obsidian allows", () => { + const doc = [ + "| Day | Task |", + "| - | :-: |", + "| Monday | spec |", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ + { table: 0, row: 1, column: 1, id: "aa11", quote: "spec", resolved: false, author: "me" }, + ]); + }); + + test("ignores a pipe line that no delimiter row follows", () => { + const doc = [ + "Shopping | list", + "eggs | milk", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test.each([ + ["outside any table", "Some prose."], + ["on the delimiter row", "| --- | --- |"], + ])("finds no cell for a range starting %s", (_label, text) => { + const doc = ["Some prose.", "", "| Day | Task |", "| --- | --- |", "| Monday | spec |"].join("\n"); + const from = doc.indexOf(text) + 2; + + expect(tableCellForRange(doc, { from, to: from + 1 })).toBeNull(); + }); + + test("places an anchor spanning two rows on the row it starts in", () => { + // It can't be painted — no one cell holds it — but its card still belongs + // beside that row rather than at the top of the table. + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | spec |", "| Tuesday | review |"].join("\n"); + const from = doc.indexOf("spec"); + const range = { from, to: doc.indexOf("review") + "review".length }; + + expect(tableCellForRange(doc, range)).toEqual({ table: 0, row: 1, column: 1, whole: false }); + expect(tableCellForRange(doc, { from, to: from + 4 })).toEqual({ + table: 0, + row: 1, + column: 1, + whole: true, + }); + }); + test("uses separate stable registry names for each color and state", () => { expect(tableHighlightName("#0090ff", false)).toBe("document-comments-table-open-0090ff"); expect(tableHighlightName("#0090ff", true)).toBe("document-comments-table-resolved-0090ff"); @@ -57,6 +143,18 @@ describe("tableHighlightTargets", () => { expect(tableHighlightName(null, false)).toBe("document-comments-table-open-default"); }); + test("gives the hovered comment its own registry name and stronger rule", () => { + // A CSS Custom Highlight carries no element, so `is-active` has nowhere to + // live — the hovered range moves to a separate name with its own rule. + expect(tableHighlightName("#0090ff", false, true)).toBe("document-comments-table-open-active-0090ff"); + expect(tableHighlightName("#0090ff", true, true)).toBe("document-comments-table-resolved-active-0090ff"); + expect(tableHighlightName("#0090ff", false, true)).not.toBe(tableHighlightName("#0090ff", false)); + + // Matches the 18% / 38% pair the DOM highlight uses for prose. + expect(tableHighlightRule("#0090ff", false, true)).toContain("color-mix(in srgb, #0090ff 38%, transparent)"); + expect(tableHighlightRule("#0090ff", true, true)).toContain("color-mix(in srgb, #0090ff 38%, transparent)"); + }); + test("renders open and resolved table colors with distinct treatments", () => { expect(tableHighlightRule("#0090ff", false)).toContain("background-color: color-mix"); expect(tableHighlightRule("#0090ff", false)).toContain("text-decoration-style: solid"); diff --git a/test/table-repair.test.ts b/test/table-repair.test.ts new file mode 100644 index 0000000..0690f05 --- /dev/null +++ b/test/table-repair.test.ts @@ -0,0 +1,329 @@ +import { describe, expect, test } from "vitest"; +import { brokenTableAnchors, computeRepairTableAnchors } from "../src/editor/table-repair"; +import { applyChanges } from "../src/editor/edits"; +import { parseComments } from "../src/format/parse"; + +const body = (id: string, quote: string) => + [`"].join("\n"); + +const repair = (doc: string) => { + const result = computeRepairTableAnchors(doc); + if (result.isErr()) throw new Error(result.error); + return applyChanges(doc, result.value); +}; + +describe("brokenTableAnchors", () => { + test("finds an anchor that displaced a row's leading pipe", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("finds an anchor that displaced the header's trailing pipe", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | spec |", body("aa11", "x")].join( + "\n", + ); + const withOpen = doc.replace("| Day", "| Day"); + + expect([...brokenTableAnchors(withOpen)]).toEqual(["aa11"]); + }); + + test("leaves a healthy in-cell anchor alone", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "spec"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("does not flag prose that merely starts with a pipe", () => { + // The line looks damaging, but no table appears when the markers come off, + // which is what the block-scoped confirmation is for. + const doc = ["Some prose.", "", "| not a table | at all", body("aa11", "x")].join( + "\n", + ); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("still finds a broken anchor beside an orphaned comment", () => { + // An orphan keeps its body after its anchored text is deleted, so it has no + // markers in the block at all. It has nothing to move and must not stop the + // anchors that do from being found. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "deleted text"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("offers no repair while a comment in the table has lost one marker", () => { + // Stripping a lone marker would delete it with nothing to put back. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "review"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("finds an anchor that filled the blank line above a table", () => { + // A triple-click selects a paragraph AND its newline, so the close marker + // used to land on the blank line a table needs above it. + const doc = [ + "Intro text", + "", + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "Intro text"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("does not flag a code comment's own-line markers", () => { + const doc = [ + "", + "```js", + "const a = 1;", + "```", + "", + "| Day | Task |", + "| --- | --- |", + '", + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("is empty for an empty document", () => { + expect([...brokenTableAnchors("")]).toEqual([]); + expect(computeRepairTableAnchors("").isOk()).toBe(true); + }); + + test("is empty for a document with no comments", () => { + expect([...brokenTableAnchors("| Day |\n| --- |\n| Monday |")]).toEqual([]); + }); +}); + +describe("computeRepairTableAnchors", () => { + test("moves the anchor inside the row and restores the table", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("repairs two broken anchors in the same table", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "y"), + ].join("\n"); + + const out = repair(doc); + + expect(brokenTableAnchors(out)).toEqual(new Set()); + expect(out).toContain("| Monday | spec |"); + expect(out).toContain("| Tuesday | review |"); + }); + + test("keeps a healthy neighbour's markers exactly where they were", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "review"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Tuesday | review |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("keeps a nested anchor intact while repairing the one around it", () => { + // A legacy whole-table anchor around a healthy in-cell comment is exactly the + // shape repair exists to undo, and the two ranges nest. Reinserting anchor by + // anchor used to shift the outer end and split the inner close marker in half. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "whole table"), + body("bb22", "spec"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(out).not.toContain("| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "whole table"), + body("bb22", "spec"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("repairs past an orphaned comment and leaves its body alone", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + body("bb22", "deleted text"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(out).toContain(body("bb22", "deleted text")); + }); + + test("rewrites only the table's own block, not the paragraphs around it", () => { + const table = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + const doc = ["Before.", "", table, "", "After."].join("\n"); + const result = computeRepairTableAnchors(doc); + + expect(result.isOk() && result.value.map(({ from, to }) => [from, to])).toEqual([ + [doc.indexOf(table), doc.indexOf(table) + table.length], + ]); + }); + + test("moves a marker off the blank line above a table and restores the table", () => { + const doc = [ + "Intro text", + "", + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "Intro text"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("Intro text\n\n| Day | Task |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("keeps a literal marker inside a code span when repairing its table", () => { + // The parser skips code, so a marker written out in a code span is text, not + // an anchor. Repair has to leave it exactly as it found it. + const doc = [ + "| Syntax | Meaning |", + "| --- | --- |", + "| `` | opens a comment |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| `` | opens a comment |"); + expect(out).toContain("| Monday | spec |"); + }); + + test("keeps markers a stray backtick hides from the parser when repairing their table", () => { + const doc = [ + "| Day | Note | Code |", + "| --- | --- | --- |", + "| Monday | spec | x |", + "| Tuesday | it`s good | `code` |", + body("aa11", "x"), + body("bb22", "good"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Tuesday | it`s good | `code` |"); + expect(out).toContain("| Monday | spec | x |"); + }); + + test("leaves the body block and thread untouched", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + + expect(repair(doc)).toContain(body("aa11", "x")); + }); + + test("writes nothing when there is nothing to repair", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "spec"), + ].join("\n"); + const result = computeRepairTableAnchors(doc); + + expect(result.isOk() && result.value).toEqual([]); + }); + + test("honors an id filter", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + const result = computeRepairTableAnchors(doc, new Set(["other"])); + + expect(result.isOk() && result.value).toEqual([]); + }); +});