From 4b3e6a6cfd865ff5f7a56d66a453ed0236f196ce Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Fri, 4 Sep 2026 22:42:21 +0200 Subject: [PATCH] refactor: move selection surgery into rich-text-dom.ts (RD-21) deleteAdjacentChip and insert did getSelection()/Range work inside the component, which pushed it over the max-lines budget under a disable comment. rich-text-dom.ts already owns the DOM boundary, so the surgery moves there as two new exports, chipAtCaret and insertChipAtCaret, and the component keeps only its event-handling and output concerns. adjacentChip stays exported with its own spec case. The component disable comment is gone, since the file is now under the line budget. Co-Authored-By: Claude Sonnet 5 --- .../readable-codebase/RD-21-rich-text-dom.md | 166 ++++++++++++++++++ docs/project/readable-codebase/README.md | 2 +- libs/shared/docs/behaviour-spec.mdx | 10 +- .../ui/rich-text-editor/rich-text-dom.spec.ts | 104 ++++++++++- .../src/ui/rich-text-editor/rich-text-dom.ts | 32 ++++ .../rich-text-editor.component.ts | 27 +-- 6 files changed, 313 insertions(+), 28 deletions(-) create mode 100644 docs/project/readable-codebase/RD-21-rich-text-dom.md diff --git a/docs/project/readable-codebase/RD-21-rich-text-dom.md b/docs/project/readable-codebase/RD-21-rich-text-dom.md new file mode 100644 index 0000000..f53266b --- /dev/null +++ b/docs/project/readable-codebase/RD-21-rich-text-dom.md @@ -0,0 +1,166 @@ +# RD-21 — Move the selection surgery into `rich-text-dom.ts`, and delete the disable + +Status: done +Source: PLAN.md 3h, order step 3 + +## Why + +`rich-text-editor.component.ts` measures ~256 effective lines against a limit of 250, so it +carries `/* eslint-disable max-lines */`. The lines that put it over are not component +concerns: `deleteAdjacentChip` and `insert` do `getSelection()`/`Range` surgery inside a +component whose job is the toolbar and the `contenteditable` host. + +`rich-text-dom.ts` already exists beside it, already owns the DOM boundary, and already has a +spec. The seam is built. This is the cheapest of Phase 3's seven splits, and it converts two +untested imperative branches into spec cases. + +## Read first + +- `libs/shared/src/ui/rich-text-editor/rich-text-dom.ts` — the four exports today + (`renderInto`, `createChip`, `readBlock`, `adjacentChip`) and the file's header comment, + which already states the contract this ticket extends. +- `rich-text-editor.component.ts:257-293` — `deleteAdjacentChip` and `insert`, the two bodies + that move. +- `rich-text-dom.spec.ts` — 8 cases, plain jsdom, no TestBed. The new cases join it. + +## Decisions (pre-made, don't relitigate) + +1. **Two new exports in `rich-text-dom.ts`, both taking the editor root:** + + ```ts + /** The chip a collapsed caret sits next to, or null. `direction` is -1 for + Backspace and 1 for Delete. Returns null when the selection is absent, is a + range rather than a caret, or sits outside `root`. */ + export function chipAtCaret(root: HTMLElement, direction: -1 | 1): HTMLElement | null; + + /** Insert `chip` at the caret when the selection is inside `root`, and leave the + caret after it. With no usable selection, append to the last line instead. */ + export function insertChipAtCaret(root: HTMLElement, chip: HTMLElement): void; + ``` + +2. **`chipAtCaret` wraps `adjacentChip`; it does not replace it.** `adjacentChip` stays + exported and keeps its own spec case, which tests the node/offset arithmetic directly. + `chipAtCaret` adds the selection guards around it. The component stops importing + `adjacentChip` and imports `chipAtCaret` instead. + +3. **The two component methods reduce to their component concerns:** + + ```ts + private deleteAdjacentChip(e: KeyboardEvent) { + const el = this.editorEl()?.nativeElement; + if (!el) return; + const chip = chipAtCaret(el, e.key === 'Backspace' ? -1 : 1); + if (!chip) return; + e.preventDefault(); + chip.remove(); + this.emit(); + } + + protected insert(key: string) { + const el = this.editorEl()?.nativeElement; + if (!key || !el) return; + el.focus(); + insertChipAtCaret(el, createChip(el.ownerDocument, key, this.labelFor(key), this.autoFor(key))); + this.emit(); + } + ``` + + `e.preventDefault()`, `chip.remove()`, `el.focus()` and `this.emit()` stay in the component: + they are event handling and output, not DOM boundary work. + +4. **Delete `/* eslint-disable max-lines */` from line 1 of the component.** This is not + optional bookkeeping — `reportUnusedDisableDirectives` is `error`, so leaving a directive + that is no longer needed **fails the build**. The two checks pin each other: if the file is + still over budget, lint fails on `max-lines`; if it is under and the directive stays, lint + fails on the unused directive. + +5. **No new file, no new folder.** `rich-text-dom.ts` is the right home and already carries the + header comment that describes exactly this responsibility. + +6. **No story changes.** `rich-text-editor.stories.ts` exercises the component through the same + public surface; nothing it renders changes. + +## Files + +- `libs/shared/src/ui/rich-text-editor/rich-text-dom.ts` — two new exports +- `libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts` — new cases +- `libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts` — two shrunken methods, + changed import, and the disable deleted +- `libs/shared/docs/behaviour-spec.mdx` (regenerated, never hand-edited) + +## Steps + +1. Add `chipAtCaret` and `insertChipAtCaret` per decision 1, moving the bodies out of the + component rather than rewriting them. +2. Add spec cases. Cover, at minimum: `chipAtCaret` returns the chip before a Backspace caret; + returns null for a non-collapsed selection; returns null for a caret outside `root`; + `insertChipAtCaret` splices at the caret and leaves the caret after the chip; + `insertChipAtCaret` appends when there is no selection inside `root`. +3. Rewrite the two component methods per decision 3 and fix the import line. +4. Delete the disable (decision 4). +5. Run `npm run gen:behaviour-spec`. +6. `git add -A`, then run the acceptance commands. +7. Update this ticket's `Status:` to `done` and the README's RD-21 row to `done`. +8. Commit all of it together. + +## Acceptance criteria + +Measured against the tree before handover. + +```bash +git grep -c "^export function" libs/shared/src/ui/rich-text-editor/rich-text-dom.ts # is 4 -> MUST be 6 +git grep -c "eslint-disable max-lines" -- libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts # is 1 -> MUST be 0 +``` + +The selection surgery has left the component entirely: + +```bash +git grep -c "getSelection" -- libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts # is 2 -> MUST be 0 +git grep -c "getSelection" -- libs/shared/src/ui/rich-text-editor/rich-text-dom.ts # is 0 -> MUST be 2 +git grep -c "adjacentChip" -- libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts # is 2 -> MUST be 0 +``` + +`adjacentChip` survives with its spec case (decision 2): + +```bash +git grep -c "export function adjacentChip" libs/shared/src/ui/rich-text-editor/rich-text-dom.ts # MUST be 1 +git grep -c " it(" libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts # is 8 -> MUST be >= 13 +``` + +```bash +npm run ci --full # exits 0 +``` + +## Verification + +**Do not add a line-count command.** `npm run lint`, inside the gate, is the exact check and a +hand-rolled `grep -v | wc -l` is not: it cannot reproduce eslint's `skipComments` for a trailing +comment or for the component's inline template. Decision 4 explains why lint alone pins both +directions. + +**`--full` is required.** This edits `libs/shared/src/ui/**`, which the README's rule names +explicitly. + +jsdom supports `document.getSelection()`, `Range.deleteContents()`, `insertNode`, +`removeAllRanges` and `addRange`, so every new case runs in the existing plain-vitest setup. No +TestBed, no browser. + +## Out of scope + +- Splitting the component further. It is over by a handful of lines, not structurally wrong. +- Touching `renderInto`, `readBlock` or `createChip`. +- The `ponytail:` note in `rich-text-dom.ts`'s header about exotic pasted markup. That is a + recorded limitation, not this ticket's work. + +## Risks + +- **Deleting the disable is mandatory, not cosmetic** (decision 4). Forgetting it fails lint + with `Unused eslint-disable directive`, which reads like an unrelated error. +- **Keep `adjacentChip` exported.** Its spec case imports it directly; folding it into + `chipAtCaret` deletes a test that covers node/offset arithmetic the wrapper does not. +- **Move the bodies, do not rewrite them.** The caret placement after insert + (`setStartAfter` → `collapse(true)` → `removeAllRanges` → `addRange`) is the part users feel; + a "cleaner" rewrite is where a regression hides, and no story catches it. +- **`behaviour-spec.mdx` drift** from the new spec titles. Regenerate in the same commit. A name + used in a `describe` or `it` title also lands in that generated file — count it if you add a + grep for one. diff --git a/docs/project/readable-codebase/README.md b/docs/project/readable-codebase/README.md index 20468df..187786f 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -115,7 +115,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | RD-18 | Ticket-reference sweep, frontend — 181 refs, 100 files | 01 | yes | done | | RD-19 | Ticket-reference sweep, backend — 370 refs, 86 files | 01 | | done | | RD-20 | `wizard-errors.ts` + spec, adopted by all 3 wizards | 02 | | done | -| RD-21 | `rich-text-dom.ts` helpers + spec cases | 02 | yes | todo | +| RD-21 | `rich-text-dom.ts` helpers + spec cases | 02 | yes | done | | RD-22 | `intake-wizard` to 3 step components | 08, 20 | yes | todo | | RD-23 | `registratie-wizard` to 3 steps + the upload-controller move | 08, 20 | yes | todo | | RD-24 | `concepts.page` to 6 sections + `concept-card` + globals + code tokens | 02 | yes | todo | diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 26cd4ee..c2b6254 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 538 frontend behaviours across +**is** the suite, reshaped for a business reader. 543 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -758,6 +758,14 @@ classes. - redirects an anonymous user to /login without waiting for caps - reads authentication through the port, not an app-local store +#### chipAtCaret / insertChipAtCaret (selection surgery) + +- chipAtCaret returns the chip before a Backspace caret +- chipAtCaret returns null for a non-collapsed selection +- chipAtCaret returns null for a caret outside root +- insertChipAtCaret splices at the caret and leaves the caret after the chip +- insertChipAtCaret appends when there is no selection inside root + #### createDebouncedSave - flushes after the delay when canSave is true diff --git a/libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts b/libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts index aeca58c..587c032 100644 --- a/libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts +++ b/libs/shared/src/ui/rich-text-editor/rich-text-dom.spec.ts @@ -1,6 +1,12 @@ -import { describe, it, expect } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; import { RichTextBlock } from '@shared/kernel/rich-text'; -import { adjacentChip, readBlock, renderInto } from './rich-text-dom'; +import { + adjacentChip, + chipAtCaret, + insertChipAtCaret, + readBlock, + renderInto, +} from './rich-text-dom'; const labelFor = (key: string) => ({ naam: 'Naam', datum: 'Datum' })[key] ?? key; @@ -131,3 +137,97 @@ describe('rich-text DOM boundary', () => { expect((chips[1] as HTMLElement).dataset['auto']).toBe('false'); }); }); + +describe('chipAtCaret / insertChipAtCaret (selection surgery)', () => { + // jsdom's Selection only accepts ranges over nodes attached to the live document, + // so every case here mounts its `root` under document.body and unmounts it after. + let mounted: HTMLElement[] = []; + + afterEach(() => { + for (const el of mounted) el.remove(); + mounted = []; + document.getSelection()?.removeAllRanges(); + }); + + function mount(root: HTMLElement): HTMLElement { + document.body.appendChild(root); + mounted.push(root); + return root; + } + + function setCaret(node: Node, offset: number): void { + const sel = document.getSelection(); + if (!sel) throw new Error('no selection in this environment'); + const range = document.createRange(); + range.setStart(node, offset); + range.collapse(true); + sel.removeAllRanges(); + sel.addRange(range); + } + + function makeChip(): HTMLElement { + const chip = document.createElement('span'); + chip.dataset['phKey'] = 'naam'; + return chip; + } + + it('chipAtCaret returns the chip before a Backspace caret', () => { + const root = mount(document.createElement('div')); + const chip = makeChip(); + const after = document.createTextNode('bb'); + root.append(chip, after); + setCaret(after, 0); + expect(chipAtCaret(root, -1)).toBe(chip); + }); + + it('chipAtCaret returns null for a non-collapsed selection', () => { + const root = mount(document.createElement('div')); + const chip = makeChip(); + const text = document.createTextNode('bb'); + root.append(chip, text); + const sel = document.getSelection(); + if (!sel) throw new Error('no selection in this environment'); + const range = document.createRange(); + range.setStart(text, 0); + range.setEnd(text, 1); + sel.removeAllRanges(); + sel.addRange(range); + expect(chipAtCaret(root, -1)).toBeNull(); + }); + + it('chipAtCaret returns null for a caret outside root', () => { + const root = mount(document.createElement('div')); + const other = mount(document.createElement('div')); + const text = document.createTextNode('bb'); + other.appendChild(text); + setCaret(text, 0); + expect(chipAtCaret(root, -1)).toBeNull(); + }); + + it('insertChipAtCaret splices at the caret and leaves the caret after the chip', () => { + const root = mount(document.createElement('div')); + const text = document.createTextNode('aabb'); + root.appendChild(text); + setCaret(text, 2); // caret between "aa" and "bb" + const chip = makeChip(); + insertChipAtCaret(root, chip); + expect(root.textContent).toBe('aabb'); + expect(Array.from(root.childNodes)).toContain(chip); + const sel = document.getSelection(); + if (!sel) throw new Error('no selection in this environment'); + const range = sel.getRangeAt(0); + expect(range.collapsed).toBe(true); + expect(root.childNodes[range.startOffset - 1]).toBe(chip); + }); + + it('insertChipAtCaret appends when there is no selection inside root', () => { + const root = mount(document.createElement('div')); + const p = document.createElement('p'); + p.textContent = 'line'; + root.appendChild(p); + document.getSelection()?.removeAllRanges(); + const chip = makeChip(); + insertChipAtCaret(root, chip); + expect(p.lastChild).toBe(chip); + }); +}); diff --git a/libs/shared/src/ui/rich-text-editor/rich-text-dom.ts b/libs/shared/src/ui/rich-text-editor/rich-text-dom.ts index 55faa95..cf2899f 100644 --- a/libs/shared/src/ui/rich-text-editor/rich-text-dom.ts +++ b/libs/shared/src/ui/rich-text-editor/rich-text-dom.ts @@ -186,6 +186,38 @@ export function adjacentChip( return isChip(sibling) ? sibling : null; } +/** + * The chip a collapsed caret sits next to, or null. `direction` is -1 for + * Backspace and 1 for Delete. Returns null when the selection is absent, is a + * range rather than a caret, or sits outside `root`. + */ +export function chipAtCaret(root: HTMLElement, direction: -1 | 1): HTMLElement | null { + const sel = root.ownerDocument.getSelection(); + if (!sel || !sel.isCollapsed || !sel.rangeCount) return null; + const range = sel.getRangeAt(0); + if (!root.contains(range.startContainer)) return null; + return adjacentChip(range.startContainer, range.startOffset, direction); +} + +/** + * Insert `chip` at the caret when the selection is inside `root`, and leave the + * caret after it. With no usable selection, append to the last line instead. + */ +export function insertChipAtCaret(root: HTMLElement, chip: HTMLElement): void { + const sel = root.ownerDocument.getSelection(); + if (sel && sel.rangeCount && root.contains(sel.anchorNode)) { + const range = sel.getRangeAt(0); + range.deleteContents(); + range.insertNode(chip); + range.setStartAfter(chip); + range.collapse(true); + sel.removeAllRanges(); + sel.addRange(range); + } else { + (root.lastElementChild ?? root).appendChild(chip); + } +} + function markOf(el: HTMLElement): Mark | null { switch (el.tagName) { case 'STRONG': diff --git a/libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts b/libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts index 844b08e..eafd04f 100644 --- a/libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts +++ b/libs/shared/src/ui/rich-text-editor/rich-text-editor.component.ts @@ -1,7 +1,6 @@ -/* eslint-disable max-lines */ // toolbar + contenteditable logic in one component — removed by RD-21 import { Component, ElementRef, computed, effect, input, output, viewChild } from '@angular/core'; import { RichTextBlock, emptyBlock } from '@shared/kernel/rich-text'; -import { adjacentChip, createChip, readBlock, renderInto } from './rich-text-dom'; +import { chipAtCaret, createChip, insertChipAtCaret, readBlock, renderInto } from './rich-text-dom'; /** A menu entry for the insert-placeholder control — a plain {key,label}, so the editor stays domain-free (it never sees the brief's PlaceholderDef). */ @@ -257,15 +256,7 @@ export class RichTextEditorComponent { private deleteAdjacentChip(e: KeyboardEvent) { const el = this.editorEl()?.nativeElement; if (!el) return; - const sel = el.ownerDocument.getSelection(); - if (!sel || !sel.isCollapsed || !sel.rangeCount) return; - const range = sel.getRangeAt(0); - if (!el.contains(range.startContainer)) return; - const chip = adjacentChip( - range.startContainer, - range.startOffset, - e.key === 'Backspace' ? -1 : 1, - ); + const chip = chipAtCaret(el, e.key === 'Backspace' ? -1 : 1); if (!chip) return; e.preventDefault(); chip.remove(); @@ -276,19 +267,7 @@ export class RichTextEditorComponent { const el = this.editorEl()?.nativeElement; if (!key || !el) return; el.focus(); - const chip = createChip(el.ownerDocument, key, this.labelFor(key), this.autoFor(key)); - const sel = el.ownerDocument.getSelection(); - if (sel && sel.rangeCount && el.contains(sel.anchorNode)) { - const range = sel.getRangeAt(0); - range.deleteContents(); - range.insertNode(chip); - range.setStartAfter(chip); - range.collapse(true); - sel.removeAllRanges(); - sel.addRange(range); - } else { - (el.lastElementChild ?? el).appendChild(chip); - } + insertChipAtCaret(el, createChip(el.ownerDocument, key, this.labelFor(key), this.autoFor(key))); this.emit(); } }