PT-4020 Backspace confirmation#492
Conversation
When structure is unprotected, Backspace/Delete at a verse- or paragraph-marker boundary becomes a deliberate two-step gesture: the first press selects the target (verse marker -> NodeSelection, shown with a destructive-red highlight; paragraph -> whole-block range), the second press deletes it (verse removed; paragraph merged into the previous block, preserving text). No confirmation dialog. Renames StructureProtectionPlugin -> StructureKeyboardPlugin: one KEY_DOWN handler now blocks structural edits when protected (PT-4013) and two-steps them when unprotected, keyed on isStructureProtected so the two modes are mutually exclusive. A useRef latch prevents firing on hand-made selections. ImmutableVerseNode now reflects its node-selection via useLexicalNodeSelection (verse-selected class) since Lexical does not style decorators on selection; the platform stylesheet styles it with the shadcn destructive token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ors into pt-4020-backspace-confirmation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
The verse DecoratorNode's selection subscription (useLexicalNodeSelection -> isSelected -> getNodes) throws in Lexical 0.43.0 when a RangeSelection has an element-type point anchored on the verse node (the "cursor on the verse number" case). That throw surfaced as an unhandled error failing the test run. Override isSelected to treat the expected throw as "not selected", consistent with getSelectionStartNode's handling of the same selection shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename structureProtection.{utils,utils.test,baseline.test} trio to
structureKeyboard.* for consistency with StructureKeyboardPlugin
- Collapse $caretAdjacentToVerseMarker to delegate to $adjacentVerseMarker
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Session-URL: <session URL>
Enrich the two-step intentional-delete feedback for verse markers and extend it to range selections: - Model: add a "selection" ArmedDelete kind + $isArmedSelection branch that validates a latched non-collapsed range by endpoint snapshot. - Plugin: mirror armed state into React state (setArmedState); arm a range selection that contains a verse marker (first press) and delete it via selection.removeText() on the second; toggle a `verse-delete-armed` class on the editor root to gate the blink; render the tooltip when armed (except para). - Tooltip: new VerseDeleteTooltip (FloatingBox + @floating-ui/dom computePosition, no autoUpdate) with role=status/aria-live and a <kbd> keycap, plus a pure armedDeleteMessage(kind, intent) microcopy helper. - CSS: roomier no-reflow red background on .verse-selected, a caret-like glow-underline blink gated on .verse-delete-armed, a prefers-reduced-motion static-glow fallback, and destructive tooltip styling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ors into pt-4020-backspace-confirmation
captaincrazybro
left a comment
captaincrazybro
left a comment
There was a problem hiding this comment.
@captaincrazybro partially reviewed 17 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on jolierabideau).
a discussion (no related file):
Just had a few mostly documentation and style comments with one potential bug reported by Devin.
Probably should have Ira look at this as I don't have access to actually approve.
libs/shared-react/src/plugins/usj/armedDeleteMessage.ts line 10 at r1 (raw file):
kind: ArmedDelete["kind"], intent: ArmedDelete["intent"], ): { key: string; text: string } {
Add JSDoc comments for parameters? And maybe return JSDoc?
libs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 48 at r1 (raw file):
* @param isStructureProtected - When true, structural keystrokes are blocked; when false, the * two-step intentional-delete behavior is active. * @returns Always `null`; this component has no UI.
This JSDoc comment doesn't seem to be correct. Seems like it does sometimes return JSX.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts at r1 (raw file):
Should the functions in this file have JSDoc comments for the parameters and return values?
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 288 at r1 (raw file):
para.remove(); if (junction) $placeCaretAtEnd(junction); else if (moved.length > 0) $placeCaretAtEnd(prev);
From Devin:
Cursor lands at end instead of merge junction when merging into an empty paragraph
The caret is placed at the end of the merged content ($placeCaretAtEnd(prev) at structureKeyboard.utils.ts:288) when the previous paragraph was empty, so the cursor jumps to the wrong position after a two-step paragraph merge.
Impact: After merging a paragraph into an empty preceding paragraph via the two-step delete, the user's cursor unexpectedly appears at the end of the text instead of at the beginning.
Detailed mechanism: $mergeParaIntoPrevious caret placement logic
Suggested fix
else if (moved.length > 0) prev.selectStart();
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 15 at r1 (raw file):
* that the jsdom test environment lacks; we re-position on window scroll/resize instead. */ export function VerseDeleteTooltip({
JSDoc?
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 37 at r1 (raw file):
middleware: [flip(), shift()], }) .then((pos) => {
I don't know what the style guide for this repo is but should we try to use try/catch notation here instead of .then() and .catch().
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Jolie, thanks for making it clearer what might get deleted when a verse marker could be impacted! I just added a few nitpicky changes to consider. Levi did a great job with documentation related reviews that are also worth looking at. I made one change blocking just to make sure you have a chance to look at all of these before it merges.
@katherinejensen00 partially reviewed 17 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on jolierabideau).
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 44 at r1 (raw file):
*/ function VerseDecorator({ nodeKey, text }: { nodeKey: NodeKey; text: string }): ReactElement { const [isSelected] = useLexicalNodeSelection(nodeKey);
NIT Claude Finding (test hygeine)
This new selection subscription makes the test run print a bunch of not wrapped in act(...) warnings. They're harmless (tests still pass), but they're new with this PR and worth silencing so the suite stays clean.
What's happening: VerseDecorator now subscribes to selection here via useLexicalNodeSelection, so changing the selection re-renders it — which the old plain-<span> decorate() never did. The tests change selection via updateSelection, and that helper runs its update without an act() wrapper (unlike pressKey, which wraps), so React complains.
Easiest fix: wrap the update inside updateSelection (in libs/shared/src/nodes/usj/test.utils.ts) — or its call sites in the plugin tests, e.g. StructureKeyboardPlugin.test.tsx:432 — in act(). You've already got the right pattern in your new ImmutableVerseNode.test.tsx, which wraps its selection updates in act(), so it's just applying that here too.
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 60 at r1 (raw file):
coords={coords} className="floating-box verse-delete-tooltip" role="status"
NIT Claude Finding (Minor accessibility improvement)
Nice addition with the tooltip! Quick heads-up: a screen reader probably won't reliably read this tooltip aloud when a marker gets armed, so its accessibility benefit may not actually land yet.
The reason: the tooltip renders through FloatingBox, which sets aria-hidden on the element until it has finished positioning itself. But positioning happens asynchronously, so for the first moment the tooltip exists it's both a live region (aria-live="polite") and aria-hidden="true" — and anything aria-hidden is invisible to screen readers. By the time it un-hides, the text is already there, which is the case screen readers are least reliable about announcing.
Small fix if you want it: don't put aria-hidden on the live region (gate just the visibility/opacity on position instead), or keep one always-present region and drop the message into it when arming. Totally fine to roll into the deferred armed-state follow-up.
libs/shared-react/src/plugins/usj/structureKeyboard.baseline.test.tsx line 76 at r1 (raw file):
// With the plugin present but not in protected mode (isStructureProtected={false}), the plugin // now implements the two-step intentional-delete behavior (PT-4020). Structural Backspace is
NIT This is super nitpicky and potentially subjective, but it can be nice to include the url to the jira work item so it is easy to navigate to.
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
Looking pretty good however there is the problem of localized messages and tooltips. I wrestled for some time with how to deal with all this between this repo and core and in the end realized that we want to keep the editor repo doing editor only things and that means all user-facing things including messages, localizations, and the look-and-feel (including the decision to ShadCN or something else) belongs in core so that all those decisions and their business logic are there also.
@irahopkinson partially reviewed 15 files and all commit messages, and made 14 comments.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on jolierabideau and katherinejensen00).
libs/shared-react/src/plugins/usj/structureKeyboard.baseline.test.tsx line 76 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT This is super nitpicky and potentially subjective, but it can be nice to include the url to the jira work item so it is easy to navigate to.
This is super nitpicky on my part but I don't even like to refer to Jira issues in code since this is open source but Jira is not. Describe what is fixed without reference to Jira (you already do here). We could also change to a different issue tracker in the future and our code would still be littered with references devs can't see.
On the other hand it's fine to reference them in PRs and commit messages which is the point it makes most sense to reference them since a PR usually completes an issue.
libs/shared-react/src/plugins/usj/StructureProtectionPlugin.tsx at r1 (raw file):
FYI it's really hard to see what is changed if you rename and modify in one commit. If the modification isn't small then git will show this (like here) as a delete and add rather than a change. It also means you loose all the git history (including git blame) for the file. That doesn't really matter in this case since the file was only recently added. In the future it would be better to do all the renames in one commit and the modification in subsequent commits. You also have to retain both commits when you merge rather than squashing.
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 43 at r1 (raw file):
* {@link VERSE_SELECTED_CLASS_NAME}. */ function VerseDecorator({ nodeKey, text }: { nodeKey: NodeKey; text: string }): ReactElement {
NIT move this React component definition to below the ImmutableVerseNode class definition. That class matches the filename so we want to see that first in the file before the detail of the new component used in the class.
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 46 at r1 (raw file):
const [isSelected] = useLexicalNodeSelection(nodeKey); // ZWSPs stay inside this span so double-click word selection still excludes the number. return <span className={isSelected ? VERSE_SELECTED_CLASS_NAME : undefined}>{text}</span>;
It will get this CSS class when it's selected from this delete new trigger mechanism but won't it also get this class if it's selected for any other reason? I.e. is it possible to do node selection with the cursor or is that always a RangeSelection? Will that cause any issues?
Since I first wrote this I see you now intentionally trigger on a range selection anyway. Doesn't this CSS class only show on a NodeSelection? Is that right?
Code quote:
VERSE_SELECTED_CLASS_NAMElibs/shared-react/src/nodes/usj/ImmutableVerseNode.test.tsx line 21 at r1 (raw file):
/** Query the rendered verse marker that carries the "selected" class, if any. */ function selectedVerseEl(editor: LexicalEditor): Element | null { return editor.getRootElement()?.querySelector(`.${VERSE_SELECTED_CLASS_NAME}`) ?? null;
NIT this seems redundant since it's a null coalescing operator and querySelector returns Element | null anyway. Am I missing something? If instead you did ?? undefined that would allow you to favor undefined over null but isn't necessary in tests.
Also move helper functions like this after the tests since it is detail we don't need to read first.
Code quote:
?? nulllibs/shared-react/src/plugins/usj/armedDeleteMessage.ts line 7 at r1 (raw file):
* of the sentence. `para` is never shown a tooltip, so it maps to the verse-marker wording. */ export function armedDeleteMessage(
We can't do messages like this from the editor repo because we only do localization in core. Find a way to trigger this in core.
libs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 42 at r1 (raw file):
* boundaries. When `isStructureProtected`, blocks them (PT-4013) and sanitizes paste/drop payloads * so structural markers (paragraph breaks, verse markers, chapter markers) cannot be inserted — * text, inline character formatting, and notes are kept. When not protected, makes deletion a
BTW the unprotected state means the 2-step removal will be enabled for Power. Have you thought if that is appropriate? I think it's probably fine but you might want to check with UX (if you haven't already).
Code quote:
When not protectedlibs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 105 at r1 (raw file):
} else if (armed.kind === "selection") { if ($isRangeSelection(selection)) selection.removeText(); // deletes the whole range, verse included } else if (node && $isElementNode(node)) {
Should this be $isSomeParaNode? We shouldn't merge some other ElementNode with either of SomeParaNode.
For that matter we should think if it's ok to merge a ParaNode with an ImpliedParaNode (and vice versa). Now that I have written that I think merging either to either is fine (but not with some other ElementNode).
Code quote:
$isElementNodelibs/shared-react/src/plugins/usj/structureKeyboard.utils.ts at r1 (raw file):
BTW consider if any of these utils are generic enough to be moved to node.utils.ts or node-react.utils.ts. Also consider if there is already a utility there that would do what you need here. The /simplify command might help with this.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 172 at r1 (raw file):
/** What a structural delete keystroke would act on. */ export type DeleteTarget =
BTW it's good practice to declare types (including interfaces) at the top of the file (after imports).
Here and below also.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 174 at r1 (raw file):
export type DeleteTarget = | { kind: "verse"; node: SomeVerseNode } | { kind: "para"; node: ElementNode };
NIT seems like this should be more specific like SomeParaNode. If you really mean ElementNode then the kind probably shouldn't be "para".
Code quote:
ElementNodelibs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 15 at r1 (raw file):
* that the jsdom test environment lacks; we re-position on window scroll/resize instead. */ export function VerseDeleteTooltip({
We shouldn't do tooltips in this repo because we want tooltips (and any other component) to conform to ShadCN that we use in core (or any other library we might change to in the future). By rolling our own here (which AI has a bad propensity to do) we also would have to fix all the edge cases that are already functional in ShadCN. Instead can we add something here that would allow us to trigger a tooltip in core?
packages/platform/src/usj-nodes.css line 2269 at r1 (raw file):
} /* Verse marker armed for the two-step intentional delete (first Backspace/Delete selects it).
These changes belong elsewhere like in editor.css. In the future this file for USJ nodes will be generated for each project/resource by core so this change would get lost but we still want these changes to remain so move them. This also means we can use editor-overrides.scss in core if we want to change the colors there.
…reProtectionActive StructureKeyboardPlugin no longer renders its own tooltip. Instead it publishes the armed state onto the editor root (`verse-delete-armed` class plus `data-verse-delete-intent`/`data-verse-delete-kind`) so the host app owns the localized destructive hint. Removes VerseDeleteTooltip and armedDeleteMessage, drops the now-unused `verseKeys` from ArmedDelete, and relocates the armed CSS from usj-nodes.css into editor.css (which core does not regenerate). Also adds the `isStructureProtectionActive` option (default true): when false (e.g. Power interface mode) the plugin registers no handlers and editing is fully native. Threaded through EditorOptions, Editor.tsx, and the API report. Tightens the paragraph-merge path to use $isSomeParaNode/SomeParaNode instead of the broad ElementNode, so only paragraphs merge into paragraphs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o file end ArrowNavigationPlugin.test now imports the act-wrapped updateSelection from react-test.utils rather than reaching into shared/test.utils directly. Hoists VerseDecorator (ImmutableVerseNode) and selectedVerseEl (its test) below their first use, matching the repo's define-after convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jolierabideau
left a comment
jolierabideau
left a comment
There was a problem hiding this comment.
Agreed! The editor repo no longer renders user-facing copy/tooltip:
- Deleted
armedDeleteMessage.tsandVerseDeleteTooltip.tsx(and their tests) from shared-react. StructureKeyboardPluginnow publishes the armed state to the editor root asdata-verse-delete-intent(deleteBackward/deleteForward) anddata-verse-delete-kind(verse/selection); the armed marker already carriesverse-selected. It renders nothing.- In paranext-core, new
verse-delete-tooltip/VerseDeleteTooltipOverlayobserves those DOM signals and renders a ShadCNTooltipwith copy fromuseLocalizedStrings, anchored to the marker — built to match the existingParagraphMarkerTooltipOverlayprecedent. Wired intoplatform-scripture-editor.web-view.tsxaround the editor. New localized keys..._verseDelete_removeVerseMarker/..._verseDelete_deleteSelectionadded tolocalizedStrings.json(en + es).
@jolierabideau made 23 comments and resolved 7 discussions.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on captaincrazybro, irahopkinson, and katherinejensen00).
a discussion (no related file):
Previously, captaincrazybro (Cqptain) wrote…
Just had a few mostly documentation and style comments with one potential bug reported by Devin.
Probably should have Ira look at this as I don't have access to actually approve.
Done.
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 43 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
NIT move this React component definition to below the
ImmutableVerseNodeclass definition. That class matches the filename so we want to see that first in the file before the detail of the new component used in the class.
Done. VerseDecorator now sits after the ImmutableVerseNode class definition.
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 44 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding (test hygeine)
This new selection subscription makes the test run print a bunch of
not wrapped in act(...)warnings. They're harmless (tests still pass), but they're new with this PR and worth silencing so the suite stays clean.What's happening:
VerseDecoratornow subscribes to selection here viauseLexicalNodeSelection, so changing the selection re-renders it — which the old plain-<span>decorate()never did. The tests change selection viaupdateSelection, and that helper runs its update without anact()wrapper (unlikepressKey, which wraps), so React complains.Easiest fix: wrap the update inside
updateSelection(inlibs/shared/src/nodes/usj/test.utils.ts) — or its call sites in the plugin tests, e.g.StructureKeyboardPlugin.test.tsx:432— inact(). You've already got the right pattern in your newImmutableVerseNode.test.tsx, which wraps its selection updates inact(), so it's just applying that here too.
Done Added a React-aware updateSelection wrapper in react-test.utils.tsx that runs the selection change inside act(...), and repointed the component test files to it (the shared updateSelection stays framework-agnostic so libs/shared keeps no React-testing dependency). Two raw editor.update selection calls in structureKeyboard.utils.test.ts were routed through the existing sutUpdate helper.
libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx line 46 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
It will get this CSS class when it's selected from this delete new trigger mechanism but won't it also get this class if it's selected for any other reason? I.e. is it possible to do node selection with the cursor or is that always a
RangeSelection? Will that cause any issues?Since I first wrote this I see you now intentionally trigger on a range selection anyway. Doesn't this CSS class only show on a NodeSelection? Is that right?
verse-selected reflects whether the node is selected, so it shows both when armed (a NodeSelection on the verse) and when the verse merely falls inside an ordinary RangeSelection. A verse inside a selection gets the subtle destructive tint, but the caret-like blink is gated on the verse-delete-armed root class, so only an actually-armed marker blinks. So: tint = "this verse is in the selection", blink = "armed, press again to delete".
libs/shared-react/src/nodes/usj/ImmutableVerseNode.test.tsx line 21 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
NIT this seems redundant since it's a null coalescing operator and
querySelectorreturnsElement | nullanyway. Am I missing something? If instead you did?? undefinedthat would allow you to favor undefined over null but isn't necessary in tests.Also move helper functions like this after the tests since it is detail we don't need to read first.
Done. Dopped the ?? null and moved the helper function.
libs/shared-react/src/plugins/usj/armedDeleteMessage.ts line 7 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
We can't do messages like this from the editor repo because we only do localization in core. Find a way to trigger this in core.
Done.
libs/shared-react/src/plugins/usj/armedDeleteMessage.ts line 10 at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
Add JSDoc comments for parameters? And maybe return JSDoc?
Done Removed this to handle it in core
libs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 42 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW the unprotected state means the 2-step removal will be enabled for Power. Have you thought if that is appropriate? I think it's probably fine but you might want to check with UX (if you haven't already).
Done. Scoped it to simple only
libs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 48 at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
This JSDoc comment doesn't seem to be correct. Seems like it does sometimes return JSX.
Done Good catch, it did return <VerseDeleteTooltip>. With the tooltip moved to core the component now genuinely always returns null, so the return type is null and the JSDoc is accurate. I also expanded the JSDoc to document the DOM signals it publishes.
libs/shared-react/src/plugins/usj/StructureKeyboardPlugin.tsx line 105 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
Should this be
$isSomeParaNode? We shouldn't merge some otherElementNodewith either ofSomeParaNode.For that matter we should think if it's ok to merge a
ParaNodewith anImpliedParaNode(and vice versa). Now that I have written that I think merging either to either is fine (but not with some otherElementNode).
Done. The guard is now $isSomeParaNode(node), and $mergeParaIntoPrevious takes a SomeParaNode and also checks its previous sibling with $isSomeParaNode — so only paragraphs merge into paragraphs.
libs/shared-react/src/plugins/usj/StructureProtectionPlugin.tsx at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
FYI it's really hard to see what is changed if you rename and modify in one commit. If the modification isn't small then git will show this (like here) as a delete and add rather than a change. It also means you loose all the git history (including git blame) for the file. That doesn't really matter in this case since the file was only recently added. In the future it would be better to do all the renames in one commit and the modification in subsequent commits. You also have to retain both commits when you merge rather than squashing.
That is a great point, thanks!
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 172 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW it's good practice to declare types (including interfaces) at the top of the file (after imports).
Here and below also.
Done.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 174 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
NIT seems like this should be more specific like
SomeParaNode. If you really meanElementNodethen thekindprobably shouldn't be "para".
Done. DeleteTarget's para variant is now { kind: "para"; node: SomeParaNode }, and $structuralDeleteTarget resolves it with $isSomeParaNode.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts line 288 at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
From Devin:
Cursor lands at end instead of merge junction when merging into an empty paragraph
The caret is placed at the end of the merged content (
$placeCaretAtEnd(prev)at structureKeyboard.utils.ts:288) when the previous paragraph was empty, so the cursor jumps to the wrong position after a two-step paragraph merge.Impact: After merging a paragraph into an empty preceding paragraph via the two-step delete, the user's cursor unexpectedly appears at the end of the text instead of at the beginning.
Detailed mechanism: $mergeParaIntoPrevious caret placement logic
Suggested fix
else if (moved.length > 0) prev.selectStart();
Done Confirmed the bug: when the previous paragraph was empty (junction undefined), $placeCaretAtEnd(prev) put the caret at the end of the moved content. The logic now collapses to if (junction) $placeCaretAtEnd(junction); else prev.selectStart();, so the caret lands at the junction in both cases.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
Should the functions in this file have JSDoc comments for the parameters and return values?
Done.
libs/shared-react/src/plugins/usj/structureKeyboard.utils.ts at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW consider if any of these utils are generic enough to be moved to
node.utils.tsornode-react.utils.ts. Also consider if there is already a utility there that would do what you need here. The/simplifycommand might help with this.
I do not believe they are generic enough to move, and I could not find any in the other files that would be useful. /simplify confirmed that but did:
- Removed dead
verseKeysfield fromArmedDelete— it was only ever read by the now-deletedVerseDeleteTooltip. Dropped the field from the type, stopped populating it in StructureKeyboardPlugin.tsx, and removed it from two test fixtures. (anchor/focusare not dead —$isArmedSelectionstill reads them — so they stayed.) - Fixed stale JSDoc on
ArmedDelete.keythat still referenced "the tooltip anchor." - Restored the tighter
Element | nullreturn type on theselectedVerseEltest helper (the diff had widened it to includeundefined).
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 15 at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
JSDoc?
Done. File removed
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 15 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
We shouldn't do tooltips in this repo because we want tooltips (and any other component) to conform to ShadCN that we use in core (or any other library we might change to in the future). By rolling our own here (which AI has a bad propensity to do) we also would have to fix all the edge cases that are already functional in ShadCN. Instead can we add something here that would allow us to trigger a tooltip in core?
Done.
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 37 at r1 (raw file):
Previously, captaincrazybro (Cqptain) wrote…
I don't know what the style guide for this repo is but should we try to use
try/catchnotation here instead of.then()and.catch().
Done. File removed
libs/shared-react/src/plugins/usj/VerseDeleteTooltip.tsx line 60 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding (Minor accessibility improvement)
Nice addition with the tooltip! Quick heads-up: a screen reader probably won't reliably read this tooltip aloud when a marker gets armed, so its accessibility benefit may not actually land yet.
The reason: the tooltip renders through
FloatingBox, which setsaria-hiddenon the element until it has finished positioning itself. But positioning happens asynchronously, so for the first moment the tooltip exists it's both a live region (aria-live="polite") andaria-hidden="true"— and anythingaria-hiddenis invisible to screen readers. By the time it un-hides, the text is already there, which is the case screen readers are least reliable about announcing.Small fix if you want it: don't put
aria-hiddenon the live region (gate just thevisibility/opacityon position instead), or keep one always-present region and drop the message into it when arming. Totally fine to roll into the deferred armed-state follow-up.
Done. Fixed by migrating this to core. The hint is now a vetted ShadCN Tooltip in core rather than our own FloatingBox live region, so we no longer own the aria-hidden-while-positioning edge case. If we want guaranteed announcement of the armed state we can layer a dedicated live region in the core overlay as a follow-up — flagging for the deferred armed-state work you mentioned.
packages/platform/src/usj-nodes.css line 2269 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
These changes belong elsewhere like in
editor.css. In the future this file for USJ nodes will be generated for each project/resource by core so this change would get lost but we still want these changes to remain so move them. This also means we can useeditor-overrides.scssin core if we want to change the colors there.
Done. Moved the marker styling (verse-selected, the armed blink, reduced-motion) to packages/platform/src/editor/editor.css.
libs/shared-react/src/plugins/usj/structureKeyboard.baseline.test.tsx line 76 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
This is super nitpicky on my part but I don't even like to refer to Jira issues in code since this is open source but Jira is not. Describe what is fixed without reference to Jira (you already do here). We could also change to a different issue tracker in the future and our code would still be littered with references devs can't see.
On the other hand it's fine to reference them in PRs and commit messages which is the point it makes most sense to reference them since a PR usually completes an issue.
Done. Removed Jira reference from code
…ors into pt-4020-backspace-confirmation
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
Thanks for fixing this all up. Just one blocker (notified in 2 places).
@irahopkinson partially reviewed 19 files and all commit messages, made 5 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on captaincrazybro, jolierabideau, and katherinejensen00).
libs/shared-react/src/nodes/usj/ImmutableVerseNode.test.tsx line 21 at r1 (raw file):
Previously, jolierabideau wrote…
Done. Dopped the
?? nulland moved the helper function.
err, it still has the ?? null.
libs/shared-react/src/plugins/usj/react-test.utils.tsx line 9 at r2 (raw file):
} from "../../../../../packages/utilities/src/converters/usj/converter-test.data"; // eslint-disable-next-line @nx/enforce-module-boundaries import { updateSelection as $updateSelection } from "../../../../../libs/shared/src/nodes/usj/test.utils";
NIT $ is special in Lexical signifying that this function must be called in an editor.update() or editor.read() so use something else here for the local name. A common pattern we use in core would be to rename $updateSelection to updateSelectionInternal or internalUpdateSelection.
Consider if the original function in test.utils.ts could be internally wrapped in act() and would that work every where it's already called.
Code quote:
$updateSelectionpackages/platform/src/editor/Editor.tsx line 145 at r2 (raw file):
isReadonly = false, isStructureProtected = false, isStructureProtectionActive = true,
Shouldn't this default to false so it works for Power and Simple explicitly sets it true?
packages/platform/src/editor/editor.model.ts line 197 at r2 (raw file):
* Whether the structure-protection feature applies at all. When false (e.g. Power interface * mode), the structural keyboard handling — both protection-blocking and the two-step * intentional delete — is disabled and editing is fully native. Defaults to active (true).
I think this should default to false (i.e. Power).
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson made 1 comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on captaincrazybro, jolierabideau, and katherinejensen00).
packages/platform/src/editor/editor.model.ts line 193 at r2 (raw file):
isReadonly?: boolean; /** When true, paragraph and verse markers cannot be changed via keyboard input. */ isStructureProtected?: boolean;
Instead of having 2 booleans how about one tri-state property, i.e. structureProtectionMode: 'off' | 'protected' | 'protectedAnd2StepRemove'; - you can probably think of better state names.
Code quote:
isStructureProtected4 participants
Code Review Summary
Branch: pt-4020-backspace-confirmation
Base: origin/main
Date: 2026-07-01
Review model: Claude Opus 4.8
Files changed: 10
Overview
This branch makes deleting structural verse/paragraph markers a deliberate two-step
gesture, so they can no longer be removed with a single accidental keystroke. When the
editor is not in structure-protected mode, the first Backspace/Delete at a marker
boundary "arms" the target — a verse marker is put into a
NodeSelectionand renderedwith destructive styling (
verse-selected), while a paragraph is selected as a range.A second matching keypress fires the delete (removing the verse marker, or merging the
paragraph into the previous one); a mismatched-direction key, arrow, click, or typing
clears the armed latch instead. The former
StructureProtectionPluginwas renamed toStructureKeyboardPluginsince it now governs both PT-4013 structure protection and thenew two-step delete. Supporting work includes a
VerseDecoratorthat reflects nodeselection, a
VERSE_SELECTED_CLASS_NAMEexport, a themeable.verse-selecteddemostylesheet rule, and an
ImmutableVerseNode.isSelectedoverride that guards against anexpected Lexical element-point
getNodes()throw.API Changes
shared-react(libs/shared-react/src/plugins/usj): RemovedStructureProtectionPluginexport (file deleted, no longer re-exported fromplugins/usj/index.ts).shared-react(libs/shared-react/src/plugins/usj): AddedStructureKeyboardPluginexport (new fileStructureKeyboardPlugin.tsx, re-exported fromplugins/usj/index.ts). Same props shape as the removed plugin:{ isStructureProtected: boolean }, returnsnull.shared-react(libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx): AddedVERSE_SELECTED_CLASS_NAMEexport ("verse-selected").shared-react(libs/shared-react/src/nodes/usj/ImmutableVerseNode.tsx): AddedisSelected(selection?: BaseSelection | null): booleanoverride onImmutableVerseNode(matches Lexical base signature; non-breaking refinement).structureKeyboard.utils.tsgained new helpers ($structuralDeleteTarget,$isArmedSelection,$placeCaretAtEnd,$mergeParaIntoPrevious,DeleteTarget,ArmedDelete, etc.). Not part of the public API surface.Findings
Critical — Must address before merge
Public export(Author: paranext-core does not import this plugin by name; it enables it through an editor option, so no external consumer depends on the export name. Not a real breaking change.)StructureProtectionPluginrenamed toStructureKeyboardPlugininlibs/shared-react/src/plugins/usj/index.ts— a breaking change for any external consumer importing the old name.[Author response: Dismissed — the plugin is not consumed by name externally; consumers enable it via an editor option, so the rename is behavior- and consumer-safe.]
Important — Should address before merge
Stale build artifacts ((Verifiedlibs/shared-react/dist/,out-tsc/) still reference the removedStructureProtectionPlugin; a stale.d.tscould mask the rename ifdist/is published.dist/andout-tsc/are gitignored and untracked — not in the diff or committed anywhere; a clean rebuild regenerates them.)border-radius: 2px), with no non-color affordance — an accessibility gap for color-vision-deficient users and high-contrast themes. (Deferred: author will address armed-state affordances holistically in a follow-up.).verse-selectedstyling, but an armed paragraph is shown via a plain native range selection that looks identical to ordinary text selection — no "next press is destructive" signal. (Deferred: making the paragraph case look destructive is non-trivial — paragraphs aren't decorator nodes, so it would require switching paragraph arming to aNodeSelectionand teachingParaNodeto render a destructive class, with some risk to the existing range-based merge/caret logic. Author chose to defer to the follow-up.)aria-live/ ARIA state), so non-sighted users get no signal that the next keypress will delete. (Deferred: there is no existingaria-live/announcement pattern anywhere in the codebase — all 13 aria usages are static attributes — so adding one is net-new accessibility infrastructure, folded into the follow-up.)[Author response: The stale-artifact finding was dismissed after verifying the paths are gitignored/untracked. The three UX/accessibility findings (color-only cue, inconsistent verse-vs-paragraph feedback, no SR announcement) are acknowledged as legitimate but intentionally deferred to a dedicated follow-up that will handle armed-state affordances holistically; #4 is non-trivial and #5 has no existing pattern to extend.]
Minor — Consider
StructureKeyboard*, but the support trio kept the oldstructureProtection.*stem. (fixed during review:git mvrenamedstructureProtection.utils.ts/.utils.test.ts/.baseline.test.tsxtostructureKeyboard.*; updated the import inStructureKeyboardPlugin.tsx, the import +describelabel instructureKeyboard.utils.test.ts, and a comment reference inStructureKeyboardPlugin.test.tsx.)$caretAdjacentToVerseMarkerand$adjacentVerseMarkerinstructureKeyboard.utils.tsreimplemented the same boundary-resolution logic line-for-line. (fixed during review: collapsed$caretAdjacentToVerseMarkertoreturn !!$adjacentVerseMarker(selection, direction);, making$adjacentVerseMarkerthe single source of truth.).verse-selectedinpackages/platform/src/usj-nodes.css:2269-2279uses literal hex reds (#dc2626) rather than a theme token.--destructiveas HSL components that can't be referenced directly; the platform app rebinds.verse-selectedto the live--destructivetoken in paranext-core's_usj-nodes.scss. Consistent with other hardcoded colors in the demo stylesheet.)VERSE_SELECTED_CLASS_NAME = "verse-selected"(ImmutableVerseNode.tsx:36) — a clean, documented addition; noted only that host apps may now depend on the string value, so treat it as a stable contract going forward. (FYI, no action.)[Author response: Requested and received fixes for the two style items (file rename + DRY collapse). The demo-CSS hardcoded-color note was dismissed as intentional/documented; the new public class-name constant is an accepted FYI.]
Template Propagation
Shared Regions Modified
None. (No
#region shared with <url>markers exist in any changed file.)Extension Config Changes
None. (This is the
scripture-editorsNx monorepo — noextensions/tree.)Other Propagation Notes
packages/platform/src/usj-nodes.css:2269-2279— The demo.verse-selectedrule is the counterpart of styling the platform app maintains in paranext-core's_usj-nodes.scss(bound to the live--destructivetoken, per the in-file comment). If the armed/selected verse-marker visual treatment changes here, the paranext-core rule may need a matching update. This is a code-comment-documented cross-repo coupling, not a formal shared-region marker.Positive Observations
clearStaleLatch).if (intent !== armed.intent) return true;).StructureKeyboardPluginpreserves the exact props contract ({ isStructureProtected: boolean }) andnullreturn of the plugin it replaces — the migration is a one-line rename for consumers.ImmutableVerseNode.isSelectedmatches Lexical's base signature exactly and reuses the existing publicisSelectionStartNodeExpectedErrorhelper rather than duplicating error-detection logic; the guard only swallows the one known-benign element-point throw and re-throws anything else.decorate()refactor intoVerseDecoratoris purely internal rendering;SerializedImmutableVerseNodeand all top-level exports are unchanged.structureKeyboard.utils.test.ts(46 cases) exercises boundary detection and target resolution;StructureKeyboardPlugin.test.tsx(21 cases) covers the protected-blocking path and the arm/fire/cancel flow incl. mismatched-direction cancel and stale-latch guards;ImmutableVerseNode.test.tsx(3 new cases) covers selected-class rendering and the element-point edge case.<StructureKeyboardPlugin />is correctly placed in the alphabetical plugin block inEditor.tsx, honoring the CLAUDE.md ordering rule.Interview Notes
StructureProtectionPlugin→StructureKeyboardPluginrename is not a breaking change for real consumers. No deprecation alias needed.aria-livepattern in the codebase to extend.isSelectedoverride: Author asked for a walkthrough of the override rather than explaining it unprompted, then confirmed the explanation matched their mental model. The reasoning:ImmutableVerseNodeis aDecoratorNodewhoseVerseDecoratorsubscribes viauseLexicalNodeSelection, which callsisSelected→ basegetNodes(), which throws on aRangeSelectionwith an element-type point on the decorator (caret on the verse number). The override catches only that expected throw (matched viaisSelectionStartNodeExpectedError) and returnsfalse, re-throwing anything else; atry/catchwas chosen because the node must remain aDecoratorNode. Author confirmed this matches their understanding — no unresolved gap, though a reviewer may still want to spot-check this subtle area.In-Review Quality Check
Two minor findings were fixed during the review (file-trio rename to
structureKeyboard.*, and collapsing$caretAdjacentToVerseMarkerto delegate to$adjacentVerseMarker). Quality checks onshared-reactafter the changes:nx typecheck shared-react— passed clean. (The editor's transientdescribe/it/expect/vi"cannot find name" diagnostics on the renamed test files did not appear in the real typecheck — confirmed spurious tsserver artifacts.)nx lint shared-react— 0 errors. 3 pre-existing warnings in unrelated files (filterAndRankItems.ts,selection.utils.data-driven.test.ts) left untouched.nx test shared-react— 1076 passed / 3 skipped across 19 test files; the renamedstructureKeyboard.*tests ran and passed.No fixes beyond the requested changes were required.
Suggested Review Focus
Prioritized areas for the author-reviewer meeting:
StructureProtectionPlugin→StructureKeyboardPluginrename is safe: verify paranext-core (and any other consumer) truly enables the plugin via an editor option and never imports it by name.aria-liveinfrastructure).ImmutableVerseNode.isSelectedelement-point guard: confirmisSelectionStartNodeExpectedErrorreliably identifies only the benign throw and that no other selection shape is silently swallowed.AI-assisted — session
This change is