[jolierabideau]

Code Review Summary

Branch: pt-4960-delete-empty-para-after-chapter

Base: origin/main

Date: 2026-10-29

Review model: Claude Opus 5.5

Files changed: 8

Overview

With structure unprotected, an empty paragraph right after \c could never be deleted. Deleting a paragraph merges it into the one before, and after a chapter there is nothing to merge into. This branch lets such a paragraph be removed when another paragraph follows it in the chapter. In "guarded" the delete takes two presses: the first arms it and shows the removeEmptyPara hint, the second removes it. In "off" it takes one. The caret moves to the paragraph that follows. An empty paragraph that is alone in its chapter (the next chapter or the end of the book after it) leaves a blank line with no marker, per Ian's answer on PT-4960. Protected structure is unchanged.

The branch also stops both delete keys from pulling text under an empty paragraph's marker. Delete inside an empty paragraph, and (added during this review) Backspace at the start of the paragraph after one, now remove the empty paragraph. The paragraph with text keeps its own marker. Before, verse text could silently become heading text.

During the review, the arm and fire steps of a paragraph delete were made to agree on direction. $paraDeleteIntent decides, at arm time, from where the caret was, whether the second press removes forward (the caret moves to the next paragraph) or joins backward (the caret stays put). This fixed a caret jump when Delete at the end of a paragraph removed an empty paragraph below it.

API Changes

The published platform-editor API is unchanged (no etc/*.api.md changes). The changes below are in the internal workspace library libs/shared-react, re-exported through src/plugins/usj/index.ts:

• Added $canDeletePara(para: SomeParaNode): boolean
• Added $deletePara(para: SomeParaNode, intent?: DeleteIntent): void
• Added $getEmptyParaBefore(para: SomeParaNode): SomeParaNode | undefined (added during review)
• Added $paraDeleteIntent(selection: BaseSelection, para: SomeParaNode, intent: DeleteIntent): DeleteIntent (added during review)
• ArmedDelete gains an optional paraIntent?: DeleteIntent field (added during review)
• $canRemoveEmptyParaAfterChapter was exported on the branch and made module-private during review
• $canMergeIntoPrevious and $mergeParaIntoPrevious are still exported, but the plugins now call them through $deletePara
• ParaDeleteEffect is unchanged. "removeEmptyPara" already existed, and paranext-core already localizes it (en and es), so paranext-core needs no code change.

Findings

Critical — Must address before merge

None.

Important — Should address before merge

• Delete at the end of a non-empty paragraph moved the caret into the paragraph after an empty next paragraph (structureKeyboard.utils.ts $deletePara, StructureKeyboardPlugin.tsx fire path). The arm step targeted the next, empty paragraph for a merge. The fire step then took the new remove branch and placed the caret at the start of the paragraph after it, contradicting the "a Delete arm keeps the caret" design. (fixed during review: the new $paraDeleteIntent settles the direction at arm time and stores it on ArmedDelete.paraIntent, which the fire step passes to $deletePara. Regression tests added: the caret stays at first:5, and a selected-marker Delete gives the same result in "guarded" and "off".)
• Backspace and Delete gave different results at the same empty-paragraph boundary. Delete in an empty \s kept the following \p's marker. Backspace at the start of that \p still merged the verse text under \s. (fixed during review: Backspace at a paragraph's start now targets an empty paragraph right before it ($getEmptyParaBefore) and removes it. The text paragraph keeps its marker and the caret. The rule is applied in "guarded" ($structuralDeleteTarget) and in "off" (ParaMarkerSelectionPlugin's caret path). A selected marker deliberately still merges up. Tests added for both modes, with and without a gutter marker.)
• The off-mode Backspace-from-caret path wasn't tested for the empty paragraph after \c. (fixed during review: added a test showing that one backward delete from a caret in the empty \s removes it and lands the caret in the \p.)
• Stale TSDoc and comments still described merge-only behaviour. This covered $structuralDeleteTarget, $paraDeleteEffect, $isUnmergeableParaStart, the $mergeParaIntoPrevious dispatch note, the StructureKeyboardPlugin plugin TSDoc and its arm, fire and refusal comments, and ParaMarkerSelectionPlugin's $deferToTwoStepDelete and class TSDoc. (fixed during review: all of them reworded. The $isUnmergeableParaStart rename was deliberately deferred; its doc now explains that it means "no paragraph delete may act here".)

The author agreed with each finding and asked for the recommended fix.

Minor — Consider

• Delete on a selected marker of an empty mid-chapter paragraph put the caret in different places in "guarded" (next paragraph) and "off" (previous paragraph). (fixed during review: resolved by the $paraDeleteIntent change above, with a test covering both modes)
• The PR description overstates off-mode scope. In "off", a forward Delete from a caret in an empty paragraph is still native, so the next paragraph's text comes up under the empty paragraph's marker. Only the selected-marker and Backspace paths changed in "off". (fixed during review: PR description corrected)
• $canRemoveEmptyParaAfterChapter was exported but used only inside its file. (fixed during review: made module-private)
• expectCaretIn read selection.anchor; CLAUDE.md says to read focus. (fixed during review: it reads focus now)
• The "Delete in an empty paragraph" test duplicated blocksOf inline and had no caret assertion. (fixed during review: blocksOf and expectCaretIn hoisted to file scope and reused; caret assertions added)
• Unit test name style (structureKeyboard.utils.test.ts). (fixed during review: renamed to "deleteForward in an empty para with a next para → the empty para itself", and a sibling test added for the new Backspace target)
• Test gaps. (fixed during review: added tests for two empty paragraphs after \c (removed one at a time, caret moving down), Undo restoring a removal in one step, Delete in an empty paragraph right before a \c (left alone), and caret checks in the after-chapter ParaMarkerSelectionPlugin tests)
• The committed packages/platform/dist wasn't verified byte-for-byte. Permissions blocked the analysis pass from running the rebuild and verify scripts, and the source changed during the review anyway. (fixed during review: rebuilt with scripts/rebuild-committed-output.mjs after the source commit; scripts/verify-committed-dist.mjs reports "Committed build output is current")
• A refused delete gives no feedback (a paragraph with text after \c, protected mode). The silent refusal predates this PR. A refusal hint would need a new data-verse-delete-kind value and new localized strings in paranext-core, so it's recorded as a follow-up rather than done here.

The author asked to fix all the minor findings. All were addressed except the refusal-feedback follow-up, which needs a paranext-core change.

Template Propagation

Shared Regions Modified

None. This repo has no templates to propagate to.

Extension Config Changes

None. paranext-core already maps the reused removeEmptyPara kind.

Positive Observations

• Both plugins go through one $canDeletePara predicate and one $deletePara operation, so arming, refusal and ParaMarkerSelectionPlugin's defer decision stay in agreement.
• The removal rule is deliberately narrow. Only an empty paragraph is ever removed, so no text can be lost, and an empty paragraph alone in its chapter is replaced by a blank line with no marker, so the chapter still has a line to type in.
• The two-press safeguard is kept for the new removal in "guarded", and "off" stays one press, so each mode is consistent with itself.
• New helpers sit next to their analogues, and every new $ function documents its calling context, as CLAUDE.md requires. There are no issue-tracker IDs in code or comments.
• The tests follow house style: the bare $createParaNode is hoisted with children chained inside $getRoot().append(...), and checks read editor-state JSON rather than the DOM. They cover the negative cases as well (next chapter, end of book, protected mode).
• Reusing the existing removeEmptyPara kind avoids any localization or paranext-core change.
• The PR description sets out each decision made while waiting on product input, with the exact code to change if the answer differs.

Interview Notes

• Purpose: let an empty paragraph right after \c be deleted, prompted by an empty GNTD-based book (1 Samuel 1) and a Discord report. Taken from the PR description; the author passed the PR URL as the review argument.
• Important Adopt this repository into the paranext organization #1 (caret jump): the author asked for a recommendation and accepted it: decide the direction once, at arm time, and reuse it at fire time. Two regression tests were added; both fail without the fix.
• Important Remove the scribe package #2 (Backspace/Delete asymmetry): the author chose to extend the rule to Backspace rather than list it as a known asymmetry for Ian. The empty paragraph is now the armed target, so the highlight and hint point at what actually goes. A selected marker deliberately keeps merge semantics, because Backspace on a paragraph's own marker means "remove this marker".
• Important Flaky interaction tests: ScriptureReferencePlugin and marker-palette each failed once in CI #3 (off-mode test): the author asked whether the test was recommended, then accepted it. While writing it, a related off-mode test turned out to have only appeared to pass. Its caret check failed after act returned because jsdom's DOM selection re-synced Lexical's selection. Tracing showed the handler leaves the caret in the right place, so the test now captures the caret from the delete's own committed update, as other tests in that file do.
• Important Consider committing built dist/ like platform-bible-react, so consumers stop building at install time #4 (stale docs): the author agreed to reword everything and defer the $isUnmergeableParaStart rename.
• Design explanation (Step 3.3): the author explained that a paragraph delete always acts on one target paragraph, and that two questions about it are kept separate: what happens to it (merge or remove), and which way the caret goes. The caret direction is decided once, at arm time, from where the caret was, and the fire step reuses that decision rather than working it out again. This is accurate and matches the code ($paraDeleteIntent → ArmedDelete.paraIntent → $deletePara).
• Minor findings: the author said "fix all". The refusal-feedback item was recorded as a follow-up because it needs paranext-core strings.
• Wrap-up: the author had nothing further to flag.
• Note for the reviewer: for most findings the author asked for, and accepted, the reviewer's recommendation rather than proposing their own approach. Their Step 3.3 explanation shows they understand the resulting design.

In-Review Quality Check

• pnpm nx run-many -t typecheck lint test -p shared-react platform: all targets pass. Lint reports 4 warnings, all in files this PR doesn't touch. Tests: 2317 passed, 1 skipped in the run shown, and all 2180 usj tests pass.
• The first run's platform typecheck failed with TS6305 ("output file has not been built from source"). Platform's typecheck read dist declaration files while the build was rewriting them, a build-order race unrelated to these changes. The re-run passed.
• Build noise (packages/utilities/dist, a stray packages/platform/dist/Editorial.d.ts) was restored or removed and isn't committed.
• The committed platform dist is rebuilt and verified after the source commit, because its sourcemap embeds the Prettier-formatted source.

Suggested Review Focus

• $paraDeleteIntent (structureKeyboard.utils.ts): check its rules (forward only when the caret already sits where removal would leave it; a selected marker is always backward) against every arm path: Backspace and Delete, with and without a gutter marker, in "guarded" and "off".
• The Backspace extension changes existing behaviour. Backspace at the start of any paragraph after an empty paragraph, not only after \c, now removes the empty one instead of merging into it. Confirm this is wanted, and add it to the questions for Ian next to "Decisions" item 4.
• Manual PT10 check, "off" mode: Backspace at the start of a paragraph after an empty one. The caret's final position was verified only at the commit of the edit, because jsdom re-syncs the selection afterwards.
• Manual PT10 check from the PR: in an empty GNTD-based book, 1 Samuel 1, put the caret in the empty \s and press Backspace twice. The \s should go and the caret should land in the \p.
• Follow-up ticket: feedback when a delete is refused (new data-verse-delete-kind value and localized string in paranext-core).
• Off mode: forward Delete from a caret in an empty paragraph is still native. Is that acceptable, or should off mode get the forward rule too?

---

PT-4960: Let an empty paragraph after a chapter be deleted

Problem

With structure unprotected (Simple mode, "guarded"), deleting a paragraph merges it into the one before it. Right after a \c there is nothing to merge into, so the delete was refused. That also meant an empty line right after \c could never be removed. Roopa hit this with the empty \s in an empty book based on the GNTD model (1 Samuel 1), and Sebastian hit it with an empty row after \c on Discord.

What changed

• An empty paragraph right after \c can now be deleted with the usual two presses, when another paragraph follows it in the chapter. The first press highlights it and the second removes it. The caret goes to the start of the next paragraph. This works by Backspace or Delete from the caret, or with its marker selected.
• Delete in an empty paragraph, or Backspace at the start of the paragraph after one, now removes the empty paragraph. Before, either key joined the two paragraphs under the empty paragraph's marker. In the reported book, that would have turned the verse text into an \s heading. The paragraph with text keeps its marker, and the caret stays where it was. A selected marker still merges its paragraph up, as before.
• Power mode ("off") gets the same removal in one press, by Backspace from the caret or with the marker selected. It had the same dead end. A forward Delete from the caret is still native in "off", so there, Delete in an empty paragraph still pulls the next paragraph's text up under its marker.
• An empty paragraph that is the only one in its chapter (the next chapter or the end of the book follows it) is replaced by a blank line with no marker, with the caret in it, after the same two presses (one in "off"). The user gives it a marker with the \ menu or the paragraph-style dropdown (formatPara). Backspace in the blank line does nothing.
• A \ menu paragraph pick at the start of a line with no marker now gives that line the marker and keeps its text. Before, the pick split the line into another marker-less line and gave up, so nothing happened.
• A chapter with no line after it (the next chapter or the end of the book follows it) now loads with the same blank line, in views where markers aren't editable text. Before, a click below the chapter number usually put the caret before the chapter, so typed text landed in front of \c (or in the \id line for chapter 1). The blank line saves as nothing, so the file doesn't change.
• Protected structure is unchanged: still refused.
• Standard view is unchanged. None of the removal or blank-line behavior applies there, because the marker is editable text (review feedback).

The first press reports data-verse-delete-kind="removeEmptyPara", which paranext-core already maps to its localized tooltip ("Press {key} again to remove the empty paragraph"). No paranext-core code change is needed, only the usual dist update.

Product decisions

Ian answered Q1–Q3 on PT-4960. Items 1–3 follow his answers. Items 4 and 5 weren't asked and remain our call.

1. Allow the two-press delete of an empty paragraph after \c (Q1). Ian: yes. This follows NN 8 (paragraph-level markers can be added, deleted or changed easily when permitted), and it keeps the deliberate-delete safeguard from nice-to-have 7.

   • If no: revert $canRemoveEmptyParaAfterChapter's use in $canDeletePara (structureKeyboard.utils.ts).

2. Deleting the only paragraph in a chapter leaves a blank line with no marker (Q2). Ian: "If it is possible deleting the final marker should be possible, just leaving a blank area where a user should insert a marker." The editor can't leave nothing at all, because in Simple mode the caret can't sit on a chapter number, so the user would have nowhere to add a marker. The paragraph is replaced by an empty ImpliedParaNode ($canBlankEmptyParaAloneInChapter and $deletePara in structureKeyboard.utils.ts), and the \ menu now retags such a line (markerMenuApply.utils.ts). The first press reuses the existing removeEmptyPara hint.

   • Reload: an empty blank line saves as nothing (the chapter's USJ has no paragraph). The editor now adds the same blank line on load after any chapter with no line after it (insertBlankLinesAfterBareChapters in usj-editor.adaptor.ts), so a session and a reload look the same. Delta positions agree too: the blank line counts as one LF in both, and the host only applies delta ops to the editor's own live state, never to file offsets. Annotation positions are USJ paths, where an empty implied paragraph has no items, so they don't shift.

3. A paragraph with text right after \c is still refused (Q3). Ian: yes, keep blocking until the paragraph is empty. A future floating toolbar may offer to change the marker instead.

4. Delete in an empty paragraph, and Backspace at the start of the paragraph after one, remove the empty paragraph, everywhere, not only after \c (not asked). This was a related trap in the same scenario, and it changes existing behavior for empty paragraphs between two paragraphs. Backspace was added in review so the two keys give the same result at the same boundary.

   • If unwanted outside the \c case: in $structuralDeleteTarget, drop the backward branch's $getEmptyParaBefore target, and in the forward branch also require the empty paragraph to follow a chapter. In ParaMarkerSelectionPlugin's off-mode caret path, drop the $getEmptyParaBefore branch.

5. Power mode ("off") also removes the empty paragraph (not asked). It happens in one press, as other merges do there.

   • If unwanted: make ParaMarkerSelectionPlugin's $removeMarker and off-mode gutter path call $mergeParaIntoPrevious again.

Testing

• New tests in StructureKeyboardPlugin.test.tsx cover:
  • Backspace and Delete arm-then-remove, with and without a gutter marker, and caret placement
  • a blank line left when the paragraph is alone in its chapter or at the end of the book, Backspace refused in that blank line, and refusal under protected structure
  • Delete in an empty paragraph, and Delete at the end of the paragraph before one (the caret stays put)
  • Backspace after an empty paragraph
  • two empty paragraphs after \c, Undo, and Delete before a \c
• New tests in ParaMarkerSelectionPlugin.test.tsx cover:
  • the selected marker under "guarded" (two presses) and "off" (one press)
  • a selected marker's Delete giving the same caret in both modes
  • an "off" backward delete from the caret, both after \c and after an empty paragraph
• New unit tests in structureKeyboard.utils.test.ts cover the target and refusal rules.
• New tests in gutterParaMarkers.test.tsx cover a bare chapter loading with a blank line that saves as nothing, its delta matching the delta after deleting the chapter's only paragraph, and an annotation in the next chapter landing on the right text. usj-editor-adaptor.test.ts checks that the blank line is added in paragraph-structure view and left out in Standard view.
• guardedMergeTyping.test.tsx is back to main's expectation (Standard view unchanged).
• nx run-many -t test typecheck lint -p shared-react platform passes, and verify-committed-dist.mjs passes.

Manual check in PT10: in an empty GNTD-based book, go to 1 Samuel 1, put the caret in the empty \s and press Backspace twice. The \s goes, and the caret lands in the \p below. In a chapter whose only paragraph is an empty \s, press Backspace twice. A blank line with no marker is left. Type \ there and pick \p; the line becomes a \p. Undo the pick, save, and reopen the chapter: the blank line is still there, and clicking below the chapter number puts the caret in it. Also, in Power mode, press Backspace at the start of a paragraph that follows an empty one. The empty paragraph goes, and the caret stays at the start of the text.

🤖 Generated with Claude Code

---

This change is 

[tjcouch-sil]

Thanks, Jolie! I traced the arm and fire steps through guarded, off and protected modes, and they agree on direction in every case I tried. I'd like two changes before this merges (marked [Required] inline), and a few [Strongly suggested] ones. Two of those aren't tied to a line:

[Strongly suggested] CHANGELOG. packages/platform/CHANGELOG.md has no entry for this, and Backspace/Delete behave differently in the paragraph-structure view now. Could you add a short line under Unreleased → Fixed? For example: an empty paragraph right after a chapter can now be deleted, and deleting an empty paragraph next to another paragraph removes the empty one instead of moving the other paragraph's text under its marker.

[Strongly suggested] Full-editor tests. Everything here is tested in shared-react with hand-built trees. packages/platform/src/editor/gutterParaMarkers.test.tsx already mounts the real <Editor> the way Simple mode runs it (paragraph-structure view, guarded), presses keys through the DOM, and can check getUsj() and undo(). Could you add these cases there, or in a file next to it? Each one should check the saved USJ, and that a single Undo restores the original:

1. Backspace twice in an empty \s right after \c 1, with a \p after it: \s is gone and \p is unchanged.
2. Delete twice in an empty \s between a \p and a \q1: \s is gone, and \q1 keeps its marker and verse.
3. Backspace twice at the start of that \q1: same result as 2.
4. An empty \s that is the only paragraph after \c 1: Backspace is refused and the USJ doesn't change.
5. Once the arm fix below is in, one Backspace at the start of \q1 after an empty \s, then a typing key: the caret is still in \q1, not in \s.

I ran 1–3 against this branch in that harness and they pass, so this should be quick to add. Please leave out an Undo check for Backspace inside an empty \s in the middle of a chapter. That goes through the existing merge path, whose Undo has a known issue that's being fixed separately, so the test would fail for reasons unrelated to this PR.

[jolierabideau]

Both non-inline items are in:

• CHANGELOG: entry added under Unreleased → Fixed.
• Full-editor tests: cases 1–5 are in gutterParaMarkers.test.tsx, each checking the saved USJ, with a single-Undo check for 1–3. There is no Undo case for Backspace inside a mid-chapter \s, as you asked.

One behavior change to flag: the "empty paragraph before" rule applies in Standard view too, so two Backspaces at the start of \q1 after an empty \p now remove the empty \p rather than merging \q1 into it. guardedMergeTyping.test.tsx is updated to match (the typed text still lands as content, now under \q1), and the CHANGELOG entry isn't limited to one view.

[tjcouch-sil]

One required follow-up on the new blank-line behaviour; details inline.

[tjcouch-sil]

Approving, thanks! I re-tested the reload case in Chromium and it behaves well now. Merge when you're ready.