From 4c1b6320290ffb808364adb7f912c0475bba3857 Mon Sep 17 00:00:00 2001 From: marc Date: Mon, 10 Aug 2026 20:00:46 +0200 Subject: [PATCH] Link the margin notes from the notes, and let them be hidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The linking went the wrong way round. A remark in the margin is short by design; when there is more to say about that passage it belongs in the notes beside the document — and it has to be possible to say *which* passage. So the pins are offered to the editor's `@` menu exactly like notes and notebooks are, labelled with their page, and following one scrolls the document to the spot and opens the remark. Pins are unhidden first: a link leading to something invisible would read as broken. A pin link is the same node with kind "pin", and is deliberately kept out of the links reported to the server. Its target is a marker inside this note's own overlay, so no note has that id, and reporting it would fill the backlink table with rows matching nothing. They can also be hidden altogether. Reading a document as it was printed is a normal thing to want; the toolbar takes the markers and the column away together and the page takes the width back. The other direction — making a note out of a remark — is removed, along with its picker and the pin's noteId: 357 lines fewer than before. The unmount flush it turned up stays, since a drawing save was genuinely being dropped by navigating away inside its debounce. 218 unit tests, 231 browser checks across eleven suites. Co-Authored-By: Claude Opus 5 --- e2e/pdf.mjs | 106 ++++++++++------- frontend/src/App.jsx | 66 +---------- frontend/src/editor/content.test.ts | 39 +++++++ frontend/src/editor/content.ts | 4 + frontend/src/editor/noteLink.js | 9 +- frontend/src/features/notes/EditorPane.jsx | 2 - frontend/src/features/pdf/PdfMargin.jsx | 115 +------------------ frontend/src/features/pdf/PdfNoteSheet.jsx | 51 +-------- frontend/src/features/pdf/PdfViewer.jsx | 87 +++++++------- frontend/src/features/pdf/PdfWorkspace.jsx | 81 +++++++++---- frontend/src/features/pdf/PinLinkPicker.jsx | 120 -------------------- frontend/src/features/pdf/pageStrokes.ts | 9 -- readme.md | 34 +++--- 13 files changed, 239 insertions(+), 484 deletions(-) delete mode 100644 frontend/src/features/pdf/PinLinkPicker.jsx diff --git a/e2e/pdf.mjs b/e2e/pdf.mjs index 488392f..4f846ae 100644 --- a/e2e/pdf.mjs +++ b/e2e/pdf.mjs @@ -633,71 +633,91 @@ try { - /* ── a remark that turns into a note ── */ + /* ── linking a margin note from the notes beside the document ── */ + // A remark in the margin is short by design. When there is more to say about + // that passage it belongs in the notes — and it has to be possible to say + // *which* passage. await p.locator("[data-pdf-pin-mode]").click(); await p.waitForTimeout(400); const pageL = await p.locator('[data-pdf-page="1"]').boundingBox(); await p.mouse.click(pageL.x + pageL.width * 0.35, pageL.y + pageL.height * 0.2); await p.waitForTimeout(800); await p.getByLabel("Pinned note").fill("Zebrastreifen nachschlagen"); - await p.waitForTimeout(400); + await p.getByRole("button", { name: "Fertig" }).click(); + await p.waitForTimeout(2500); - const notesBefore = (await api(p, "GET", "/notes")).body.length; - await p.locator("[data-pin-make-note]").click(); - await p.waitForTimeout(4000); + await p.locator("[data-pdf-toggle-notes]").click(); + await p.waitForTimeout(1200); + const panel = p.locator("[data-pdf-notes] .ProseMirror"); + await panel.click(); + await p.keyboard.type("Dazu ausführlicher: "); + await p.keyboard.type("@"); + await p.waitForTimeout(1200); - const notesAfter = (await api(p, "GET", "/notes")).body; - check(notesAfter.length === notesBefore + 1, "a remark can become a note of its own", `${notesBefore} → ${notesAfter.length}`); - - const made = notesAfter.find((n) => n.title === "Zebrastreifen nachschlagen"); - check(!!made, "titled from what was written in the margin"); + const suggestions = await p.locator("body").innerText(); check( - (await p.locator(".ProseMirror").count()) > 0, - "and the app goes straight there rather than leaving it to be found" + /Zebrastreifen nachschlagen/.test(suggestions), + "the margin notes are offered when linking from the notes", + suggestions.slice(0, 160).replace(/\n/g, " ") ); - /* The link has to read in both directions, or a note born from a passage has - nothing on it saying which passage. */ - const body = (await api(p, "GET", `/notes/${made.id}`)).body.content; - check(body.includes(pdfNoteId), "the new note carries a link back to the document", body.slice(0, 120)); - - const back = await api(p, "GET", `/notes/${pdfNoteId}/backlinks`); + await p.keyboard.press("Enter"); + await p.waitForTimeout(2500); + const linkEl = panel.locator("[data-note-link]").first(); + check((await linkEl.count()) === 1, "one can be inserted as a link"); check( - Array.isArray(back.body) && back.body.some((b) => b.id === made.id), - "so the document lists it among its backlinks", - JSON.stringify(back.body).slice(0, 120) + (await linkEl.getAttribute("data-kind")) === "pin", + "recorded as a link to a spot, not to a note", + await linkEl.getAttribute("data-kind") + ); + check( + (await linkEl.innerText()).includes("S. 1"), + "labelled with the page it points at", + await linkEl.innerText() ); - /* Back to the document: the pin now leads to the note. */ - await p.locator("[data-note-row]").filter({ hasText: "e2e-test" }).first().click(); - await p.waitForTimeout(4000); - const goto = p.locator(`[data-pin-goto="${made.id}"]`); - check((await goto.count()) === 1, "and the margin card links to it"); + /* The link must not reach the backlink table: its target is a marker in this + note's own overlay, and no note has that id. */ + const backAfter = await api(p, "GET", `/notes/${pdfNoteId}/backlinks`); check( - (await goto.innerText()).includes("Zebrastreifen"), - "showing the note's current title", - await goto.innerText() + Array.isArray(backAfter.body) && backAfter.body.length === 0, + "and is not reported as a link to a note", + JSON.stringify(backAfter.body).slice(0, 100) ); - await goto.click(); + /* Following it goes to the passage. */ + await panel.locator("[data-note-link]").first().click(); await p.waitForTimeout(2500); check( - (await p.getByLabel("Note title").inputValue()) === "Zebrastreifen nachschlagen", - "clicking it goes to the note", - await p.getByLabel("Note title").inputValue() + (await p.getByLabel("Pinned note").inputValue()) === "Zebrastreifen nachschlagen", + "following the link opens the margin note it points at", + await p.getByLabel("Pinned note").inputValue() ); - /* Back once more, and let go of the link — the note must survive that. */ - await p.locator("[data-note-row]").filter({ hasText: "e2e-test" }).first().click(); - await p.waitForTimeout(4000); - await p.locator(`[data-pin-goto="${made.id}"]`).locator("xpath=ancestor::*[@data-pin-card][1]").click(); - await p.waitForTimeout(700); - await p.locator("[data-pin-unlink]").click(); - await p.waitForTimeout(2500); - check((await p.locator(`[data-pin-goto="${made.id}"]`).count()) === 0, "the link can be let go of"); + // With the notes panel open there is no room for a margin column beside the + // page, so the remark opens in the sheet instead. Closing the panel gives the + // column back — and the same remark is still the open one. + await p.locator("[data-pdf-toggle-notes]").click(); + await p.waitForTimeout(1500); + const openCard = p.locator("[data-pin-card]").filter({ has: p.getByLabel("Pinned note") }); + check((await openCard.count()) === 1, "and it is the card that is open in the margin"); + await p.keyboard.press("Escape"); + await p.waitForTimeout(600); + + /* ── showing and hiding them ── */ + check((await p.locator("[data-pdf-show-pins]").count()) === 1, "the margin notes can be hidden"); + await p.locator("[data-pdf-show-pins]").click(); + await p.waitForTimeout(1200); + check((await p.locator("[data-pin]").count()) === 0, "hiding takes the markers off the page"); + check((await p.locator("[data-pdf-margin]").count()) === 0, "and the column with them"); + + const pageWide = (await p.locator('[data-pdf-page="2"]').boundingBox()).width; + await p.locator("[data-pdf-show-pins]").click(); + await p.waitForTimeout(1200); + check((await p.locator("[data-pin]").count()) > 0, "showing brings them back"); check( - (await api(p, "GET", "/notes")).body.some((n) => n.id === made.id), - "and the note itself is still there" + (await p.locator('[data-pdf-page="2"]').boundingBox()).width < pageWide, + "and the page gives the column its width again" ); /* ── on a phone there is no margin to write in ── */ diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index d7260e7..6fec2d1 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -34,7 +34,6 @@ import { EditorPane } from "./features/notes/EditorPane.jsx"; import { HistoryModal } from "./features/notes/HistoryModal.jsx"; import { DrawingLayer } from "./features/drawing/DrawingLayer.jsx"; import { CANON_W, CANON_H, PREVIEW_DRAWING_W } from "./features/drawing/strokes.js"; -import { serializeNoteContent } from "./editor/content"; /** Views that are not a notebook and therefore cannot hold a new note. */ const VIRTUAL_FOLDERS = ["all", "starred", "trash", "dashboard"]; @@ -280,70 +279,9 @@ export default function App() { [notes, openFolder, openNote] ); - /** - * Turns a remark in a PDF's margin into a note of its own. - * - * The new note opens with a link back to the document it came from, so the - * connection reads in both directions: the pin leads here, and this note shows - * up in the document's backlinks. Without that a note born from a passage - * would have nothing on it saying which passage. - */ - const createNoteForPin = useCallback( - async (source, title) => { - const folderId = source.folderId || notebooks[0]?.id; - if (!folderId) return null; - const note = await data.createNote(folderId); - const doc = { - type: "doc", - content: [ - { - type: "paragraph", - content: [ - { type: "text", text: "Zu " }, - { - type: "noteLink", - attrs: { noteId: source.id, label: source.title || "Dokument", kind: "note" }, - }, - ], - }, - { type: "paragraph" }, - ], - }; - data.updateNote(note.id, { - title, - content: serializeNoteContent(doc), - links: [source.id], - attachments: [], - }); - return { ...note, title }; - }, - [data, notebooks] - ); - - /** - * Opens a note that was created a moment ago. - * - * `openLinkTarget` looks the target up in the notes list to find its notebook, - * and a note created in the same tick is not in the copy that callback closed - * over — so it went nowhere at all. Here the notebook is already known. - */ - const openCreatedNote = useCallback( - (id, folderId) => { - if (folderId) setActiveFolder(folderId); - openNote(id); - }, - [openNote] - ); - const linkProps = useMemo( - () => ({ - openLinkTarget, - searchLinkTargets, - resolveLinkLabel, - createNoteForPin, - openCreatedNote, - }), - [openLinkTarget, searchLinkTargets, resolveLinkLabel, createNoteForPin, openCreatedNote] + () => ({ openLinkTarget, searchLinkTargets, resolveLinkLabel }), + [openLinkTarget, searchLinkTargets, resolveLinkLabel] ); /* ── Actions ── */ diff --git a/frontend/src/editor/content.test.ts b/frontend/src/editor/content.test.ts index 93ea93c..de64546 100644 --- a/frontend/src/editor/content.test.ts +++ b/frontend/src/editor/content.test.ts @@ -181,3 +181,42 @@ describe("docToText", () => { expect(docToText(doc, 50).length).toBeLessThanOrEqual(50); }); }); + +describe("links to a spot in a PDF", () => { + /* They use the same node with kind "pin", but the target is a marker inside + * this note's own overlay — not another note. Reporting one as a note link + * would fill the backlink table with ids that match nothing at all. */ + const doc = { + type: "doc", + content: [ + { + type: "paragraph", + content: [ + { type: "noteLink", attrs: { noteId: "pin-abc", label: "S. 3", kind: "pin" } }, + { type: "noteLink", attrs: { noteId: "note-1", label: "Andere Notiz", kind: "note" } }, + ], + }, + ], + }; + + it("are left out of the links reported to the server", () => { + expect(extractNoteLinks(doc)).toEqual(["note-1"]); + }); + + it("do not stop the real links in the same paragraph being reported", () => { + expect(extractNoteLinks(doc)).toContain("note-1"); + }); + + it("are left out even when they are the only links there are", () => { + const onlyPins = { + type: "doc", + content: [ + { + type: "paragraph", + content: [{ type: "noteLink", attrs: { noteId: "pin-x", label: "S. 1", kind: "pin" } }], + }, + ], + }; + expect(extractNoteLinks(onlyPins)).toEqual([]); + }); +}); diff --git a/frontend/src/editor/content.ts b/frontend/src/editor/content.ts index 1df945c..bd0528d 100644 --- a/frontend/src/editor/content.ts +++ b/frontend/src/editor/content.ts @@ -117,6 +117,10 @@ function walk(node: DocNode | undefined | null, visit: (n: DocNode) => void): vo export function extractNoteLinks(doc: DocNode, selfId?: string): string[] { const ids = new Set(); walk(doc, (n) => { + // A link to a spot in a PDF uses the same node with kind "pin", but its + // target is a marker inside this note's own overlay, not another note. + // Reporting it would fill the backlink table with ids that match nothing. + if (n.attrs?.kind === "pin") return; if (n.type === "noteLink" && n.attrs?.noteId && n.attrs.noteId !== selfId) ids.add(n.attrs.noteId); }); return [...ids]; diff --git a/frontend/src/editor/noteLink.js b/frontend/src/editor/noteLink.js index 1decfeb..c52967b 100644 --- a/frontend/src/editor/noteLink.js +++ b/frontend/src/editor/noteLink.js @@ -1,9 +1,12 @@ /** - * Note links — type `@` to reference another note or notebook. + * Note links — type `@` to reference another note, a notebook, or, inside a PDF, + * one of the notes pinned to its pages. * * A link is an atomic inline node carrying the target's id, so it survives * renames: the label is refreshed from the current title on render, and the id - * is what the backlink index is built from. + * is what the backlink index is built from. A `pin` link is deliberately left + * out of that index — its target is a marker in the note's own overlay, so no + * note has that id. */ import { Node, mergeAttributes } from "@tiptap/core"; @@ -68,7 +71,7 @@ export const NoteLink = Node.create({ href: "#", title: node.attrs.label, }), - `${node.attrs.kind === "notebook" ? "📓" : "📝"} ${node.attrs.label}`, + `${{ notebook: "📓", pin: "📌" }[node.attrs.kind] || "📝"} ${node.attrs.label}`, ]; }, diff --git a/frontend/src/features/notes/EditorPane.jsx b/frontend/src/features/notes/EditorPane.jsx index 22aadb6..9ad87bf 100644 --- a/frontend/src/features/notes/EditorPane.jsx +++ b/frontend/src/features/notes/EditorPane.jsx @@ -230,8 +230,6 @@ export function EditorPane({ onOpenNote={linkProps.openLinkTarget} searchLinkTargets={linkProps.searchLinkTargets} resolveLinkLabel={linkProps.resolveLinkLabel} - onCreateNoteForPin={linkProps.createNoteForPin} - onOpenCreatedNote={linkProps.openCreatedNote} /> ) : ( CARD_H + (note.noteId ? 20 : 0); - /** Cards at their anchors, pushed down where they would sit on top of each other. */ function layout(notes, pageHeight, cssHeight) { let previousBottom = -Infinity; @@ -33,14 +28,13 @@ function layout(notes, pageHeight, cssHeight) { .map((note) => { const anchor = (note.y / pageHeight) * cssHeight - 12; const top = Math.max(anchor, previousBottom + CARD_GAP); - previousBottom = top + heightOf(note); + previousBottom = top + CARD_H; return { note, top }; }); } -function Card({ T, note, top, number, open, onOpen, onChange, onRemove, link }) { +function Card({ T, note, top, number, open, onOpen, onChange, onRemove }) { const [draft, setDraft] = useState(note.text); - const [picking, setPicking] = useState(false); const areaRef = useRef(null); // Adopt text that arrived from elsewhere — another device, or an undo — but @@ -144,23 +138,6 @@ function Card({ T, note, top, number, open, onOpen, onChange, onRemove, link }) > Entfernen - {!note.noteId && ( - <> - - - - )} - {picking && ( - setPicking(false)} - onPick={(target) => { - setPicking(false); - commit(); - link.onLink(note.id, target.id); - }} - /> - )} ) : (
)} - - {/* The note this remark grew into. Shown in both states: when the card - is closed it is the whole point of the card, and when it is open it - is what stops a second note being made for the same passage. */} - {note.noteId && ( -
- - {open && ( - - )} -
- )}
); } -const linkAction = (T) => ({ - border: "none", - background: "transparent", - color: T.accent, - cursor: "pointer", - fontSize: 10.5, - padding: 0, - marginRight: 10, -}); - -export function PdfMargin({ - T, - notes, - pageHeight, - cssHeight, - openId, - onOpen, - onChange, - onRemove, - numberOf, - link, -}) { +export function PdfMargin({ T, notes, pageHeight, cssHeight, openId, onOpen, onChange, onRemove, numberOf }) { const placed = layout(notes, pageHeight, cssHeight); // Cards near the foot of a page may need more room than the page is tall. - const last = placed[placed.length - 1]; - const needed = last ? last.top + heightOf(last.note) : 0; + const needed = placed.length ? placed[placed.length - 1].top + CARD_H : 0; return (
onChange(note.id, text)} onRemove={() => onRemove(note.id)} - link={link} /> ))}
diff --git a/frontend/src/features/pdf/PdfNoteSheet.jsx b/frontend/src/features/pdf/PdfNoteSheet.jsx index 869df19..b7594be 100644 --- a/frontend/src/features/pdf/PdfNoteSheet.jsx +++ b/frontend/src/features/pdf/PdfNoteSheet.jsx @@ -12,7 +12,7 @@ import { useEffect, useRef, useState } from "react"; * The page stays visible above it, so the passage the note is about is still * there while writing about it. */ -export function PdfNoteSheet({ T, note, number, onChange, onClose, onRemove, link }) { +export function PdfNoteSheet({ T, note, number, onChange, onClose, onRemove }) { const [draft, setDraft] = useState(note.text); const areaRef = useRef(null); @@ -109,55 +109,6 @@ export function PdfNoteSheet({ T, note, number, onChange, onClose, onRemove, lin }} /> - {/* Same two ways out as the margin card: some remarks turn into a page of - their own, and the pin should lead there afterwards. */} -
- {note.noteId ? ( - - ) : ( - - )} -
- + {/* Reading the document as it was printed is a normal thing to want, and + so is having every remark in view. Both are one click away. */} + {totalPinnedNotes(ink) > 0 && ( + + )} + {/* A note about a passage belongs on the passage. */} - )) - )} - - - ); -} diff --git a/frontend/src/features/pdf/pageStrokes.ts b/frontend/src/features/pdf/pageStrokes.ts index 0de35dc..7f46eb2 100644 --- a/frontend/src/features/pdf/pageStrokes.ts +++ b/frontend/src/features/pdf/pageStrokes.ts @@ -27,15 +27,6 @@ export interface PinnedNote { x: number; y: number; text: string; - /** - * A note this pin points at. - * - * A margin card holds a remark; some remarks turn into a page of their own. - * Keeping the id here means the passage and the note that came out of it stay - * connected in both directions — the pin leads to the note, and the note is - * written with a link back, so it shows up in the document's backlinks. - */ - noteId?: string | null; } export interface PageStrokes { diff --git a/readme.md b/readme.md index 21d5f69..dd55cf2 100644 --- a/readme.md +++ b/readme.md @@ -304,7 +304,7 @@ failure distinguished by comparing an error's `.message` to a magic string. Failures now carry a typed `SyncError.code`; `.message` still holds the same string, so the remaining JavaScript callers are unaffected. -The 215 unit tests concentrate on the same places, and several exist because the +The 218 unit tests concentrate on the same places, and several exist because the bug they cover actually happened: - a queue entry that fails must stay queued, with an attempt counter @@ -332,7 +332,7 @@ docker compose -f docker-compose.dev.yml up -d --build # must be running | `search-attachments` | 12 | body-only matches, ranking, umlauts; images as blob URLs, not inline bytes | | `history` | 10 | a large deletion is snapshotted, restoring works, restoring is itself undoable | | `drawing` | 7 | strokes survive the poller, and match across four viewport widths | -| `pdf` | 80 | import, page proportions, lazy loading, and reading with the network off | +| `pdf` | 82 | import, page proportions, lazy loading, and reading with the network off | | `attachments-lifecycle` | 10 | a picture is kept while any version shows it, and freed once none does | | `conflict` | 10 | a late write wins but the state it buries survives as a version | | `export` | 21 | the downloaded bytes are real Markdown, ZIP and PNG, unpacked and read back | @@ -795,24 +795,22 @@ handling rather than assuming: notes was therefore stored as `NULL` — every note on it lost on the next save. Both now ask about strokes *and* pins. -**A remark can become a note of its own.** "Notiz anlegen" on a card makes one, -titles it from what was written, and goes there; the pin keeps its id, so the -passage leads to that note ever after. The note is created with a link back to -the document, so the connection reads in both directions and the document lists -it among its backlinks — a note born from a passage should not be a note with -nothing on it saying which passage. An existing note can be linked instead, and a -link can be let go of without touching the note. +**The margin notes can be linked from the notes.** A remark in the margin is +short by design; when there is more to say about that passage it belongs in the +notes beside the document — and it has to be possible to say *which* passage. So +the pins are offered to the editor's `@` menu exactly like notes and notebooks +are, labelled with their page, and following one scrolls the document to the spot +and opens the remark. They are unhidden first: a link that led to something +invisible would read as broken. -Two things had to be fixed for that to work at all, both of them older than the -feature: +A pin link is the same node with `kind: "pin"`, and is deliberately left out of +the links reported to the server. Its target is a marker inside this note's own +overlay, so no note has that id, and reporting it would fill the backlink table +with rows that match nothing. -- `openLinkTarget` finds a note's notebook by looking it up in the notes list, - and a note created in the same tick is not in the copy that callback closed - over — so jumping to it went nowhere. Creating one now navigates by the - notebook it already knows. -- A drawing save waits 700ms for the next stroke. Leaving the document inside - that window dropped it, which is exactly what turning a remark into a note - does. The viewer now flushes a pending write when it unmounts. +**They can be hidden.** Reading the document as it was printed is a normal thing +to want; the toolbar toggles the markers and the column together, and the page +takes the width back while they are away. They come along when the document is exported with annotations: each becomes a numbered marker plus a small box with its text, placed on whichever side of the