Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 85 additions & 32 deletions .claude/skills/resume-ckeditor/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,47 +5,100 @@ description: Resume the long-running "retire CKEditor / unify Undo" project (BL-

# Resume the CKEditor-retirement project (BL-6681)

This is a deliberately long-running project spanning many sessions and many rebases against a
moving `master`. Its state lives in `docs/retire-ckeditor/`, not in any session's memory.
This is a deliberately long-running project spanning many sessions and months of a moving `master`.
Its state lives in `docs/retire-ckeditor/`, not in any session's memory.

## If you are on a fresh clone or a different computer

**None of this project exists on `master`** — nothing may merge there until a `Version6.5` branch is
cut (see the ground rules), so the plan, the progress log, the code, and *this skill file* all live
only on the project's own branches. A checkout of `master` has no `/resume-ckeditor` at all.

So the first move is always:

```sh
git fetch origin
git branch -r | grep BL-6681 # see what exists
git checkout BL-6681-stage1-undostack # or whatever PROGRESS.md's branch table calls the tip
```

Then read `docs/retire-ckeditor/PROGRESS.md`, whose **branch table** is authoritative for which
branch is which and which one is the current working tip. That table changes as stages advance;
trust it over this file, and over any branch name you remember.

## Do this, in order

1. **Read the state.**
- `docs/retire-ckeditor/PROGRESS.md` — the live log, current phase, and next actions. Start here.
- `docs/retire-ckeditor/PLAN.md` — the staged plan (Stages 0–6). Authoritative for *what* to
do. §11 records what BL-6681 itself asks for, and what on it is already obsolete.
- `docs/retire-ckeditor/REVIEW-NOTES.md` — findings already verified and decisions already
made. **Do not re-litigate anything settled there.** If you think a settled point is wrong,
say so explicitly to the user rather than quietly changing course.
- `docs/retire-ckeditor/BEHAVIOR-INVENTORY.md` if it exists — the behaviours that must survive.

2. **Orient in git.** `git status`, `git log --oneline -15`, and note the current branch. Because
the plan deliberately avoids long-lived branches, you may well be on `master` with nothing in
flight; that is the normal resting state between stages, not a sign something was lost.

3. **Check for drift.** Other work lands on `master` continuously. Before continuing a stage,
confirm the files it touches still look the way the plan assumes — the plan cites specific
`file:line` locations, and those move. If a citation has gone stale, fix the plan text as part
of the work; a plan nobody trusts is worse than no plan.

4. **Continue from the next unchecked item** in PROGRESS.md's "Next actions". Confirm with the
user which stage to work on if more than one is plausible.
- `docs/retire-ckeditor/PROGRESS.md` — the live log, the branch table, the master-sync log, and
the next actions. **Start here.**
- `docs/retire-ckeditor/PLAN.md` — the staged plan (Stages 0–6). Authoritative for *what* to do.
**§5 is the branch strategy and it is not optional reading** — the no-merging constraint makes
several natural instincts (rebase onto master, land the stage on master) actively wrong. §11
records what BL-6681 itself asks for, and what on it is already obsolete.
- `docs/retire-ckeditor/REVIEW-NOTES.md` — findings already verified and decisions already made.
**Do not re-litigate anything settled there.** If you think a settled point is wrong, say so
explicitly to the user rather than quietly changing course.
- `docs/retire-ckeditor/DEFERRED-EDITS.md` — edits to *existing* files that finished new code is
waiting on, each with why it is safe and what proves it worked. Check whether any are now due.
- `docs/retire-ckeditor/BEHAVIOR-INVENTORY.md` — the behaviours that must survive.

2. **Orient in git.** `git status`, `git log --oneline -15`, `git branch -vv | grep BL-6681`. You
should be on one of the project's branches, not `master`. Several live branches at once is the
normal state here, not a sign something went wrong — read PROGRESS.md's branch table.

3. **Check drift, and sync if due.** `master` moves ~17 commits a day, of which ~2 touch this
project's files. PLAN.md §5.3 gives the procedure and §5.1 the measured numbers. In short:

```sh
git log <last-sync-sha>..origin/master --oneline -- \
src/BloomBrowserUI/bookEdit/js/bloomEditing.ts \
src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts \
src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts \
src/BloomBrowserUI/lib/ckeditor
```

where `<last-sync-sha>` is the newest row of PROGRESS.md's master-sync table. Sync weekly, and
always before starting a new stage. **Merge into the integration branch; never rebase it.**

Separately, the plan cites specific `file:line` locations and those drift. If a citation has gone
stale, fix the plan text as part of the work — a plan nobody trusts is worse than no plan.

4. **Continue from the next unchecked item** in PROGRESS.md's "Next actions". Confirm with the user
which stage to work on if more than one is plausible.

5. **Before ending the session** (or when you sense you are running low on context), update
`PROGRESS.md`: what you did, what you learned, the branch/PR, and a revised "Next actions".
Do this even if the work is half-finished — especially then.
`PROGRESS.md`: what you did, what you learned, the branch state, and a revised "Next actions".
Do this even if the work is half-finished — especially then. Then **push**, so the work survives a
move to another machine.

## Ground rules for this project

- **New code goes in new files** (`bookEdit/undo/`, `bookEdit/textEditor/`). Edits to existing
files should be one-line dispatches wherever possible, and as late in the plan as possible.
This is the project's whole defence against rebase pain.
- **Don't keep a long-lived branch.** Each stage is designed to be its own small, green,
flag-inert PR onto `master`.
- **Deletion commits (Stage 5) are regenerated, never rebased.** If one conflicts, throw it away
and redo it mechanically.
- **Nothing merges to `master` until a `Version6.5` branch is cut** (manager's decision,
2026-08-06), which happens once 6.5 is mostly finished. Everything below follows from that;
PLAN.md §5 is the full treatment.
- **One long-lived integration branch, `BL-6681-ckeditor`,** is the project's trunk and the only
branch that merges `master` in. Each stage is a short-lived branch off it, PR'd *into* it and
**squash-merged**, so integration carries roughly one commit per stage. Delete a stage branch once
merged and cut the next fresh, or sibling branches re-apply changes already in.
- Note this **reverses** the project's original "never keep a long-lived branch, land small PRs
promptly" rule. If you find that rule quoted anywhere, it is historical.
- **Merge, never rebase, the integration branch or any pushed/reviewed branch.** Rebasing rewrites
reviewed commits, discards their review threads, needs a force-push, and re-resolves the same
conflict on every replay. An unreviewed, unpushed stage branch may still be rebased onto
integration freely.
- **After every master sync, run the nightly by hand:**
`gh workflow run nightly.yml --ref BL-6681-ckeditor`. It is schedule-only and master-only, and it
is the only thing that runs the full C# suite and the visual-regression suite — a branch that never
merges otherwise goes months with neither.
- **New code goes in new files** (`bookEdit/undo/`, `bookEdit/textEditor/`). Edits to existing files
should be one-line dispatches wherever possible, and as late in the plan as possible. This is the
main reason a months-long branch is survivable; record any that must wait in DEFERRED-EDITS.md.
- **Deletion commits (Stage 5) are regenerated, never reconciled.** If one conflicts with an incoming
master change, throw it away and redo it mechanically against the new state.
- **Every stage boundary must be a state that could ship as-is** — green, flag-inert, no half-applied
dispatch. The merge date is set by someone else and may move.
- Build and test through the wrappers, never bare `dotnet`/`vite` — `build/agent-dotnet.sh` and
`build/agent-vite.sh` — because the developer usually has a Bloom running via `./go.sh`. See
`AGENTS.md`. Never run the full `pnpm build`.
- To see a change in the running Bloom, just edit the source (the Vite dev server pushes it in)
and observe via the `run-bloom` skill. No build.
- To see a change in the running Bloom, just edit the source (the Vite dev server pushes it in) and
observe via the `run-bloom` skill. No build.
4 changes: 2 additions & 2 deletions docs/retire-ckeditor/BEHAVIOR-INVENTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ of CKEditor's colour panel (§4.5), which supersedes A7 and A9 rather than repro
| C2 | Pasting nested **`<div>`s** does not produce divs — importantly, a div copied from another Bloom book must not arrive with its **id**, which would duplicate an id in this book | `pasteFilter` | BL-3899 | ⚠ capture, then unit + live |
| C3 | Pasting an **`<iframe>`**, `<script>`, `<style>`, `<object>` or `<embed>` produces none of them | `pasteFilter` | | ⚠ capture, then unit |
| C4 | Pasting an **`<img>`** does not embed the image in the text | `pasteFilter` | | ⚠ capture, then live |
| C5 | Pasting arbitrary styled `<span>` soup from a real web page keeps only `font-variant` and `color` | `pasteFilter` | BL-4775, BL-12357 | ⚠ capture, then unit |
| C5 | Pasting arbitrary styled `<span>` soup from a real web page keeps only `font-variant` and `color` — **intended, and NOT what happens today.** Captured 2026-09-07 (PASTE-DROP-BASELINE.md): every span attribute and style survives, because `BloomField.restoreHtmlMarkupIfNecessary` (BL-12357) mistakes every paste for an internal one and restores the unfiltered HTML — **and with it any table/iframe/img/div in the same paste (C1–C4 defeated).** The replacement must implement the intended rule. | `pasteFilter`, undone by BL-12357's `cke/id` test | BL-4775, BL-12357 | captured ✓; unit for the new sanitizer |
| C6 | Pasting `<a href>` keeps the link; other attributes on it are dropped | `pasteFilter` `a[!href]` | | ⚠ capture, then unit |
| C7 | **Dropping** any of C1–C6 from outside Bloom is filtered the same way as pasting | CKEditor's clipboard plugin routes `drop` through the same filter, `ckeditor.js:622` | | ⚠ capture, then live |
| C7 | **Dropping** any of C1–C6 from outside Bloom is filtered the same way as pasting | CKEditor's clipboard plugin routes `drop` through the same filter, `ckeditor.js:622` | | captured ✓ 2026-09-07: drop matches paste in every row (including the C5 bypass) |
| C8 | Dragging a canvas element from the toolbox onto a page still works (Bloom's own internal drag, custom `text/x-bloom-canvas-element` type) | `CanvasElementManager.ts:2069-2088` | BL-7958 (Linux) | live |
| C9 | Pasting **plain** text with several lines produces several paragraphs | `reconstituteParagraphsOnPlainTextPaste` | BL-9961 | unit |
| C10 | Bloom's own code can still write markup the paste filter would reject (audio spans with ids, `bloom-linebreak`, canvas elements) — the filter applies **only** at the clipboard/drop boundary | `config.allowedContent = true` alongside a restrictive `pasteFilter` | BL-3899 / BL-3976 (the first fix filtered everything and broke this) | unit |
Expand Down
61 changes: 61 additions & 0 deletions docs/retire-ckeditor/DEFERRED-EDITS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Deferred edits to existing files

The project's defence against rebase pain is that new code goes in new files and edits to existing
files land as late as possible (PLAN.md §5). This file is the ledger of edits that a completed stage
of *new* code is waiting on — written down at the moment the new code was designed, while the
reasoning is fresh, so that landing them later is mechanical rather than a re-derivation.

**Every entry states what the edit is, why it is safe, and what proves it worked.** Delete an entry
when its edit lands.

Line citations are as of the commit that added the entry. They drift; the surrounding code is quoted
so the right place is still findable.

---

## Stage 1 — activate the one undo stack

**New code:** `src/BloomBrowserUI/bookEdit/undo/` — `undoTypes.ts`, `UndoStack.ts`,
`legacyUndoProviders.ts`, `runUndoable.ts`, `pageFrameUndoHooks.ts`, `redoKeyBinding.ts` and their
specs.

**Entries 1a–1e landed 2026-09-07** (commit `f92383031` on `BL-6681-stage1-undostack`). What they
did, briefly, so this file still explains the shape of `workspaceRoot.ts`:

- `registerLegacyUndoProviders()` is called once at module level in `workspaceRoot.ts`.
- `handleUndo()` and `canUndo()` are delegations to `theOneUndoStack`; the four-way if-chain, its
order, and the BL-16558 markup-update calls now live in `undo/legacyUndoProviders.ts`. The stale
"*See also Browser.Undo*" comment is gone.
- `switchContentPage()` calls `pageFrameNavigating()` before touching the old frame and
`pageFrameLoaded()` in its load handler (`undo/pageFrameUndoHooks.ts`). One hook covers same-page
reloads too, because every page-frame navigation C# makes goes through `switchContentPage`.
- `handleRedo()` / `canRedo()` are exported on the workspace bundle; Ctrl+Y is bound in the page frame
by `undo/redoKeyBinding.ts`, installed from `editablePage.ts`'s ready handler, as the last resort
behind origami's and the reader tools' handlers and CKEditor's own redo.

Three things differed from the entries as originally written — recorded in PROGRESS.md
(2026-09-07): master's BL-16558 had changed `handleUndo`; `data-page-id` is never set, so the page id
is `.bloom-page`'s `id`; and ctrl+wheel zoom no longer reloads the page.

### 1f. Expose the cross-frame push

`IWorkspaceExports` (`workspaceRoot.ts`) and the global exposure object at the bottom of the file
both need whatever Stage 2 pushes with. **Do not export `push(entry)` across frames** — that would
hand page-frame code the ability to put a page-frame closure on the stack, which is exactly the
failure `undoTypes.ts` documents at length. Export a function taking *data* and let the workspace
frame build the entry. Design it with Stage 2's first real caller, not before.

### Proof it worked

- [x] `pnpm test` green; `bookEdit/undo` specs green (52 tests, 21 of them added with the edits).
- [x] **The point of Stage 1 is that nothing changes**, so the verification is behavioural, in a
running Bloom. Done 2026-09-07 with the harnesses in `liveChecks/` (see its README); results in
PROGRESS.md under that date:
- [x] Change Layout mode: a split, Ctrl+Z undoes it, Ctrl+Y redoes it, each exactly once (origami's
own handler; ours declined). The Undo button reaches `origamiUndo` through the stack.
- [x] Decodable Reader tool active: type, Undo button — the reader-tools undo runs (`tb=1`), not
CKEditor's (`ck=0`), and the markup update follows (`markup=1`).
- [x] Image: an undoable copyright change, Undo button — `imageOperationUndo` runs (`img=1`).
- [x] Text box with no reader tool: type, Undo button — CKEditor's undo runs (`ck=1`); Ctrl+Y runs
CKEditor's redo exactly once and ours declines.
- [x] The Undo button's enabled state tracked `canUndo()` in every case.
Loading