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 <noreply@anthropic.com>
This commit is contained in:
Claude Sonnet 5
@@ -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.
|
||||
Reference in New Issue
Block a user