PT-4540: Select a paragraph marker to change it (host)

[jolierabideau]

Code Review Summary

Branch: pt-4540-para-marker-selection

Base: origin/main. The review ran against the WI-14 tip (origin/pt-4488-pt-4539-paragraph-marker-dropdown, 9fe661a). WI-14 was squash-merged in the meantime, so the branch was then rebased --onto origin/main.

Date: 2026-09-29

Review model: Claude Opus 5.5

Files changed: 17 against origin/main after the rebase. At the start of the review it was 14 PT-4540 files against the WI-14 tip; the review added marker-menu.utils.ts and its test, touched character-marker-control.component.tsx, and dropped the plan doc.

Overview

This is the host (paranext-core) half of PT-4540 (Todd NN-2.3). In Simple mode, Saroj can select a paragraph marker itself by clicking its gutter glyph, and change it with the toolbar paragraph dropdown. The editor half (selection, keys, the DOM class) is in scripture-editors branch pt-4540-para-marker-selection. This PR consumes EditorRef.getSelectedParaMarker(), EditorProps.onParaMarkerMenuRequest and the psc-para-marker-selected class.

On the host side:

• restoreSelectionIfLost treats a selected marker as a live selection, so a pick retags the marker's paragraph.
• ParagraphStyleTrigger's popover is controlled through the new useParagraphMenuOpenState hook, so Enter / Alt+↓ on a selected marker opens it. Structure lock shows the lock notification; read-only or no block ignores the request; the menu closes if it becomes unavailable.
• The menu closes on pick and returns focus to the editor through returnFocusToEditor, which restores the caret before focus(). After an outside interaction it leaves focus alone.
• The selected row gets an accent fill and a 4px --foreground leading bar across the gutter, mirrored for RTL, with a contrast test.
• The keyboard catalog has an entry for Enter / Alt+↓.

The review focused on pattern conformance and on the PT-4540 Definition of Done. During the interview:

• the duplicated close-on-pick wrapper was extracted into a shared util;
• a host-side §6 invariant regression test was added;
• the ADR citation was corrected;
• the Jira ticket was corrected about where the work lands;
• the plan doc was dropped;
• all minor findings were fixed or verified.

Two follow-up bugs came out of the review. Their ticket drafts are on the author's desktop (see Suggested Review Focus).

API Changes

None. No public API surface changed: nothing under lib/platform-bible-react/, lib/platform-bible-utils/, lib/papi-dts/papi.d.ts, or extensions/src/*/src/types/*.d.ts. Internal to the platform-scripture-editor extension:

• restoreSelectionIfLost now takes getSelectedParaMarker and returns early while a marker is selected.
• New returnFocusToEditor(editor, lastFocusOutSelection).
• ParagraphStyleTrigger has three new required props: isMenuOpen, onMenuOpenChange, onReturnFocusToEditor. The popover is now controlled.
• New hook useParagraphMenuOpenState, with the exported type ParagraphMenuOpenState.
• New util wrapMarkerMenuItemsWithClose.
• It consumes two editor APIs not yet on platform-yalc: EditorRef.getSelectedParaMarker() and EditorProps.onParaMarkerMenuRequest.

Findings

Critical — Must address before merge

• The branch builds only against a local editor build. The pinned platform-yalc revision lacks getSelectedParaMarker and onParaMarkerMenuRequest. A clean checkout or CI will fail typecheck in platform-scripture-editor.web-view.utils.ts, platform-scripture-editor.web-view.tsx, and the tests that mock them. (Author: the PR stays a draft until editor PR 1 lands on platform-yalc and is taken in with npm install && npm run verify:dev-packages plus the lockfile.)

Author response: acknowledged. The PR stays a draft until the editor PR is taken in.

Important — Should address before merge

• The paragraph menu refocuses the editor differently from CharacterMarkerControl on an outside click. The character-marker onClose always runs editorRef.current?.focus(). (Checked during review: this is intentional per editor spec §10, "Closing always returns focus to the editor, unless the user interacted outside the menu". The PR description is reworded so it no longer claims to match the character-marker control on outside click. The character-marker control's bare focus() (no caret restore) may move the caret to the chapter end on an Escape close, so a follow-up ticket draft was written: ticket-character-marker-close-caret-jump.md.)
• Record the "toolbar marker menus return focus to the editor" exception in an ADR and in dismissal-patterns.mdx. (Author: no ADR entry, since the author considers it documented. Note for the reviewer: dismissal-patterns.mdx:102-112 still says to return focus to the opener, and the exception is written down in the editor spec §10, not in the Storybook guideline.)
• The SCSS comment cited adr-list-selection-on-a-dedicated-visual-channel for a row fill that ADR rejects. (Fixed during review: the comments in _usj-nodes.scss and usj-nodes-styles.test.ts now say the rule follows the ADR's dedicated channel but departs from it in two ways. It adds a fill because a paragraph row's background carries no status. The bar is a box-shadow because the gutter lies outside the box, and box-shadow has no logical form, hence the explicit RTL rule.)
• closingMarkerMenuItems was copied from CharacterMarkerControl. (Fixed during review: new marker-menu.utils.ts exports wrapMarkerMenuItemsWithClose(items, close), with TSDoc and a test file. Both controls use it.)
• DoD: keyboard selectability deferred without a filed ticket. (Author: some of the keyboard work was done in this PR: Enter / Alt+↓ open the menu, and arrows return to the text. The rest isn't scoped yet, so no follow-up ticket for now. This DoD line is knowingly left open.)
• DoD: the PR #548 boundary-normalization finding wasn't recorded on the host side. (Addressed: editor spec §8.3 records it as orthogonal. It concerns Power mode's TextNode prefixes, and Simple's decorator prefixes are skipped by boundary normalization. The PR's "Before merge" list now carries it for the invariants owner to confirm with the sign-off.)
• DoD: invariants-owner sign-off and the agreed regression test. (Partly fixed during review: added a host regression test for Standard-View-Invariants §6 in platform-scripture-editor.utils.test.ts (with a paragraph marker selected). It connects the real generateParagraphMenuListItems to the real restoreSelectionIfLost and asserts that a retag from a selected marker calls formatPara and never setSelection, with a positive control. It was revert-checked. The owner's sign-off on the spec §8.1 statement is still pending.)
• DoD: "Lands platform-bible-react (see Q10)" didn't match the diff. (Fixed during review: the PT-4540 Jira description now says the work lands in platform-scripture-editor and the engine, not platform-bible-react, in both the header and Dependencies.)
• The plan doc broke docs durability: absolute local paths, no Frozen-record fence, and unchecked boxes. (Fixed during review: .context/designs/2026-09-25-para-marker-selection-host-plan.md was dropped from the PR; the author kept a local copy.)

Author response: all Important items were resolved, dismissed with a reason, or recorded as knowingly open (keyboard follow-up, owner sign-off).

Minor — Consider

• The selected row disappeared in forced colors (Windows High Contrast). (Fixed: @media (forced-colors: active) adds outline: 2px solid Highlight to the selected row, pinned by a test.)
• The glyph on the --accent fill used --foreground instead of its semantic pair. (Fixed: the glyph now uses --accent-foreground; the row text is unchanged. Note that --accent against --background is only 1.10–1.37:1, so in practice the bar carries the selected state.)
• Contrast test: no text check, assertions on unused tokens, and duplicated tie-backs. (Fixed: added a ≥ 4.5:1 check for --accent-foreground on --accent; removed the --primary/--ring assertions and kept the reason as a comment; each token tie-back lives only in the contrast test. A shared helper is deferred until a third contrast test exists.)
• Keyboard catalog test: a tautological keys assertion and per-entry location checks. (Fixed: removed the tautological test; one sweep now checks every catalog entry's locations, and it found no stale paths.)
• Trigger test gaps. (Fixed: added an Enter pick in the cmdk list, Escape after an editor-opened menu, and a forced close when structure protection turns on. All three return focus to the editor, and all were revert-checked.)
• No story for the editor-request open path. (Fixed: new OpenedFromTheEditor story.)
• usj-nodes-styles.test.ts: the block() helper duplicated the escaping and comment stripping. (Fixed: escape and a comment-stripped declarations are hoisted and shared.)
• useParagraphMenuOpenState returned an anonymous type. (Fixed: exported ParagraphMenuOpenState with TSDoc.)
• focusEditor name clash with the \ palette's plain focus. (Fixed: the restoring callback is now restoreCaretAndFocusEditor; the palette is unchanged.)
• makeEditor stub duplicated in web-view.utils.test.ts. (Fixed: hoisted to module level.)
• The availability gate lives in two places (open={isMenuOpen && !isStructureProtected} and the hook's close effect). (No change needed: the inline gate prevents a one-render flash before the effect closes the menu, and a comment says so.)
• A \ palette apply with a marker selected might silently do nothing. (Verified, not reachable: both host palettes are gated on viewType === 'standard', Standard has no gutter paragraph markers, and a marker can only be selected by clicking a gutter glyph. Even so, applyMarkerMenuSelection collapses the marker selection before applying.)
• Insert comment does nothing, with no feedback, while a paragraph marker is selected. The engine reports onSelectionChange(undefined) for a node selection. The host then clears currentSelectionRef, tells the backend there is no selection, and insertCommentAtCurrentSelection (web-view.tsx:~1416) returns early for the hotkey, the command, and probably the context menu. (Author chose to defer: follow-up ticket draft ticket-insert-comment-noop-on-selected-para-marker.md.)
• Screen-reader announcement of the selected marker. (Engine-owned, via aria-activedescendant per spec §10; covered by the PR's VoiceOver QA item.)

Author response: asked for all minor findings to be fixed. Two were verified with no change needed, one is engine/QA, and the comment no-op was deferred to a follow-up ticket.

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• returnFocusToEditor puts the "restore before focus()" order in one tested helper, which prevents Lexical's fallback to the document end. An invocationCallOrder test pins the order.
• restoreSelectionIfLost was extended in place rather than forked.
• useParagraphMenuOpenState is small, pure state, and fully unit-tested, including "stays closed when availability returns".
• The popover's open gate prevents a one-frame flash.
• The dismissal handling is careful. An outside interaction leaves focus where the user put it. The trigger re-click, which Radix reports as an outside pointer-down, is carved out and tested. The outside-interaction flag resets on every opening, including ones the editor drives, which never go through onOpenChange.
• The onCloseAutoFocus + preventDefault + ref pattern follows repo precedent (find-filters, footnote-type-dropdown, scope-selector).
• The bar token was chosen by measured contrast. The test sweeps every real theme from themes.data.json, guards against a sweep that runs nothing, and ties back to the stylesheet, following the ADR precedent.
• RTL mirroring is correct, and a test pins the equal-specificity source order of the SCSS.
• The keyboard catalog entry follows the rule: per-OS conventions, Enter listed first, and command deliberately unset.
• Tests are about behavior (role queries, a harness that plays the web view's part).
• Comments are forward-facing, there are no new user-visible strings, and no shadcn files are touched.
• The hidden-view sync rule doesn't apply: no cross-view sync is added.

Interview Notes

• Purpose: the host half of PT-4540. The author especially wanted review of pattern conformance and completion of the Jira issue.
• Critical (editor build): the author will keep the PR a draft until editor PR 1 lands on platform-yalc.
• Outside-click focus: the author asked for the design docs and code to be checked. The editor spec (scripture-editors/docs/superpowers/specs/2026-09-23-para-marker-selection-design.md §10, from the 2026-09-25 product/UX review) settles it: closing returns focus to the editor unless the user interacted outside the menu. The author chose to reword the PR description rather than change the behavior. They asked for the character-marker control's possible caret jump to go in a ticket draft.
• ADR: the author declined an ADR entry, considering the behavior documented in the mdx. The reviewer found that dismissal-patterns.mdx still states the opposite rule, and that the exception is recorded only in the editor spec. This is worth discussing in the meeting.
• Keyboard selection: some keyboard work was done here; the remaining scope isn't known yet, so no ticket.
• PR #548 / invariants: the author asked who the owner is and what's being asked. Neither invariants doc names an owner. By git history, the host doc was written mostly by katherinejensen00, with one commit from TJ Couch. The open questions are:
  • the spec §8.1 sign-off statement (a NodeSelection with no offsets; operations act on the owning paragraph; Backspace/Delete goes through the existing $mergeParaIntoPrevious; arrows and typing are redirected; cut, copy, paste and drop are refused; no new exclusion predicate);
  • agreement on the regression test (the engine already has one; the host one was added in this review);
  • confirming PR #548 as orthogonal;
  • confirming there should be no invariants-doc entry on either side (the engine reverted its entry in d269fb5a).
• Design walkthrough (Step 3.3): the author gave a detailed, accurate written explanation of focus and selection flow in both cases:
  • the focus-out snapshot is taken synchronously and survives a marker selection because getSelection() is undefined then;
  • restoreSelectionIfLost must skip while a marker is selected, or it would restore an out-of-date caret, retag the wrong paragraph and deselect the marker;
  • the restore must run before focus(), because Lexical's focus() selects the document end when there is no selection, after which a restore would do nothing;
  • wasClosedByOutsideInteractionRef covers the outside-interaction path, the trigger re-click is carved out, and the flag resets on every opening.
  • No gaps in understanding were apparent.
• Merge conflicts: at the author's request, the branch was rebased --onto origin/main after WI-14's squash-merge. There was one conflict in platform-scripture-editor.web-view.tsx, resolved by keeping main's ContentZoomRoot wrapper (PT-4585) and adding onParaMarkerMenuRequest. The old branch is kept locally as backup/pt-4540-pre-rebase.
• Anything else: the author had nothing further to flag.

In-Review Quality Check

• Before the rebase (a subagent ran these after all in-review changes):
  • npm run typecheck: passed.
  • npm run lint: 0 errors (1 unrelated warning in platform-bible-utils).
  • Prettier on the branch's files: no changes.
  • npm test: every workspace touched by the review passed (core 6279 tests; platform-scripture-editor 102 files).
• Not fixed, and unrelated: 3 platform-bible-react Storybook test files fail at import with does not provide an export named 'compareProjectsByName'. The export exists in both the utils src and dist, so this looks like a stale Storybook vitest dependency cache (node_modules/.cache/storybook). None of the review's changes touch lib/.
• After the rebase onto origin/main: npm run typecheck passed, npm run lint exited 0, the keyboard catalog suite passed (133 tests), and the platform-scripture-editor suite passed (107 files, 3470 tests).
• Typecheck passes locally only because the staged editor is a local build of the editor branch (see Critical).

Suggested Review Focus

• Focus returns to the editor, not the opener, on pick, Escape and trigger re-click. This goes against dismissal-patterns.mdx "Restore focus on close" (WCAG 2.4.3). Is the editor spec §10 a sufficient record, or should the guideline carry an exception note?
• Outside click: the paragraph menu leaves focus alone, as the spec says, while CharacterMarkerControl still refocuses the editor. Confirm the character-marker follow-up (ticket draft ticket-character-marker-close-caret-jump.md: a bare focus() without a caret restore may move the caret to the chapter end on an Escape close).
• Insert comment silently does nothing while a paragraph marker is selected (ticket draft ticket-insert-comment-noop-on-selected-para-marker.md). Also: is updateSelectionInternal(undefined) the right signal for other listeners while a marker is selected?
• Invariants-owner sign-off on the spec §8.1 statement, the host §6 regression test added here, PR #548 as orthogonal, and no invariants-doc entry.
• Keyboard-selection DoD line is left open with no follow-up ticket, by the author's choice. Agree how PT-4540 closes against it.
• Selected-row visibility: --accent against --background is only 1.10–1.37:1, so the 4px bar is the real signal. Is that enough of the "whole-row highlight" Jira asked for? Check in manual QA across all four themes.
• Manual QA and VoiceOver (still outstanding) once the editor build is taken in.
• Ian's decision on Backspace/Delete (merge vs. refuse) is still pending. The host contingency is a single commit.

---

Draft. Not ready to merge until the gates at the bottom are cleared. CI typecheck fails until editor PR 1 is taken in on platform-yalc.

Summary

Host (paranext-core) half of PT-4540 (Todd NN-2.3): in Simple mode Saroj can select a paragraph marker itself (click its gutter glyph) and change it with the toolbar paragraph dropdown. The editor half — selection, keys, DOM class — is scripture-editors branch pt-4540-para-marker-selection (PR 1); this PR only consumes EditorRef.getSelectedParaMarker(), EditorProps.onParaMarkerMenuRequest and the psc-para-marker-selected class.

Why review this

Part of the current epic (Todd's "Simple is coherent for Saroj", must-have 2.3).

Changes

• Retag the right paragraph. restoreSelectionIfLost treats a selected marker as a live selection, so picking from the dropdown retags the marker's paragraph instead of restoring an older caret elsewhere.
• Controlled paragraph dropdown. ParagraphStyleTrigger's popover is controlled via a new useParagraphMenuOpenState hook, so Enter / Alt+↓ on a selected marker (onParaMarkerMenuRequest) opens it. Structure lock → the existing "Structure is locked" notification; read-only / no block → ignored; the menu closes and stays closed if it becomes unavailable while open.
• Close on pick, focus back to the editor. Like the character-marker control next to it, a pick, Escape or trigger re-click (and a forced close) returns focus to the editor, after restoring a lost caret. Unlike that control, this one deliberately leaves focus where the user put it after an outside interaction, per editor spec §10. The close-on-pick wrapper is now shared by both controls (wrapMarkerMenuItemsWithClose).
• Selected-row highlight. .psc-para-marker-selected gets an --accent fill and a 4px --foreground leading bar across the gutter (RTL mirrored), on its own channel beside the active-text focus box. The bar token is pinned by a contrast test (≥ 3:1 vs --background and --accent in all four themes). The block sits after the gutter-marker rules because the selected-glyph rule ties in specificity with the base glyph rule; a test pins that order.
• Keyboard catalog entry for Enter / Alt+↓ on a selected marker (no command — it only works while a marker is selected).

Decisions since the spec (product/UX review)

From the PT-4540 Jira comment (UX) and Discord (Todd):

• Change gesture is the toolbar dropdown (the NN text).
• No new arrow-key navigation — the editor dropped the ←/→ marker stops and ↑/↓ marker walk; keyboard selection is deferred to the arrow-navigation work items (PT-3997, PT-4693, PT-3571). Some keyboard work is in this PR (Enter / Alt+↓ open the menu; arrows return to the text); the remaining scope isn't known yet, so no follow-up ticket has been filed.
• Typing on a selected marker goes into the paragraph's text (editor-only).
• Backspace/Delete merges into the previous paragraph through the editor's existing Backspace path — editor-only, no host code here. If that decision reverses to "refuse with a hint", the host side is a single contingency commit kept in the plan's appendix.
• Dropdown closes on pick for consistency with other dropdowns.

⚠️ Deliberate divergence to scrutinize: Escape (and a trigger re-click) on a toolbar-opened menu returns focus to the editor, not the button — same as the character-marker control, not Radix's default.

Hidden-tab case (cross-view-sync rule): not applicable — no cross-view sync is added, and the menu request comes only from a keypress in the focused editor.

Testing

• npm run typecheck, npm run lint: clean. Extension suite 3132/3132, catalog suite 31/31 on the rebased branch. Full npm test passed before the rebase; two workspace failures seen once (vitest worker fetch timeout; Storybook browser connection closed) passed when re-run alone.
• Revert checks: removing each key implementation line makes its named test fail (restore skip, close-on-pick, outside-interaction flag, lock guard, close-when-unavailable, contrast bar token).
• Manual QA in the app (plan Task 6, Step 4) — not yet done. Requires rebuilding the staged editor from PR 1's tip first (git -C dev-packages/scripture-editors pull && npm run build:editor).
• VoiceOver: note what is announced via the editor root's aria-activedescendant (spec §4.9).

Before merge

• WI-14 / PT-4539 merged; this PR is rebased onto main
• Editor PR 1 merged and taken in (platform-yalc + lockfile, plan Task 0 Phase B)
• Invariants-owner sign-off (spec §8.1), including the host §6 regression test added in review
• PR #548 boundary normalization: recorded as orthogonal (editor spec §8.3); invariants owner to confirm with the sign-off
• Ian's answer on Backspace/Delete (merge vs. refuse)
• Manual QA above
• Reviewer question (spec §8.5): does "a gutter marker is a NodeSelection target, with focus returned to the editor after the menu" warrant an Architecture-Decisions.md entry or an amendment to adr-list-selection-on-a-dedicated-visual-channel?

Risk Level

Medium — changes focus handling of a toolbar popover used on every paragraph retag; behaviour is covered by component tests but not yet exercised in the app.

AI Involvement

AI-assisted — planned and implemented with Claude Code (subagent-driven: per-task implementer + reviewer, then a whole-branch review whose findings were fixed). Reviewed by the author before merge.

🤖 Generated with Claude Code

---

This change is