Skip to content

[P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages)#2278

Closed
rolfheij-sil wants to merge 43 commits into
ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026from
ai/feature/markers-checklist/followup-ux2-rolf-05-12-2026
Closed

[P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages)#2278
rolfheij-sil wants to merge 43 commits into
ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026from
ai/feature/markers-checklist/followup-ux2-rolf-05-12-2026

Conversation

@rolfheij-sil

@rolfheij-sil rolfheij-sil commented May 15, 2026

Copy link
Copy Markdown
Contributor

Note: This PR replaces closed #2264 — the original was inadvertently auto-closed by GitHub's branch-rename API when the branch was renamed to follow the new follow-up convention (ai/feature/{name}/followup-{tag}-{first}-{date}). No reviews or comments had been posted before the rename. All commits are preserved on the new branch.

  • Old branch: ai/feature/markers-checklist-rolf-05-12-2026-ux2
  • New branch: ai/feature/markers-checklist/followup-ux2-rolf-05-12-2026

Merge sequencing: This PR is the third in a 3-PR follow-up to PR #2219:

Merge after #2254. Once #2254 merges, retarget this PR's base back to ai/main (GitHub does this automatically when the head branch is merged into the base branch). Once #2256 also merges, this PR may need a rebase to resolve conflicts in platform-scripture.d.ts and checklist.web-view.tsx.

The paired ai-prompts PR (design + plan + perf-profile) is paranext/ai-prompts#266.

Summary

Follow-up PR for the merged markers-checklist work (PR #2219). Addresses Sebastian's UX review delivered 2026-05-05 (UX Review of Porting Result - Markers Checklist.csv): 22 findings + 4 improvement ideas (deferred per design).

Design + implementation plan + perf-profile live in ai-prompts:

  • .context/features/markers-checklist/working-docs/ux-followup-design.md
  • .context/features/markers-checklist/working-docs/ux-followup-implementation-plan.md
  • .context/features/markers-checklist/working-docs/perf-profile.md (WP9)

This is the Sebastian UX-2 follow-up; the TJ-review follow-up lives in PR #2254.


7 commits across 9 work packages

WP Commit Findings Status
WP1 fdf9f176f6 #3, #13 (backend C#) Done
WP2 6068e92905 #19, #15, #16, #17 (UI bugfixes; #18 deferred to WP4) Done
WP3 8aa80f398f #1, #5, #6, #14 (toolbar restructure) Done
WP4 no commit #7, #18 N/A (already satisfied on HEAD via PR #2219)
WP5 b686a2a81f #8 (view-toggle dropdown) Done
WP6 07668b5acf #12 (menu rework + Notification toasts) Done
WP7 91ce56a97b #22 (persistence) Done
WP8 e6d686f5b0 #10, #11, #4 (table polish) Done
WP9 ai-prompts #9 (perf profile only — fixes in follow-up PR) Profile captured, fixes deferred

Per-finding status (Sebastian's 22)

# Finding Status Evidence
1 Two toolbars — drop inner Done (WP3) Visual: only outer chrome hamburger remains; count("[aria-label='Project']") == 1.
2 ScrollGroup vs Scope Selector integration Deferred (out of scope per design) Tracked as future ticket.
3 "Identical markers" message when no comparatives Done (WP1) Backend gates the variant on comparativeTextIds.Count > 0; UI now shows "Select a project to view its paragraph markers."
4 Header full-cell tooltip + hover Done (WP8) tw-w-full tw-cursor-default hover:tw-bg-accent/50 div wraps the cell; tooltip "rolf test" verified on hover.
5 Truncating primary placeholder Done (WP3) Primary trigger now shows short project name ("ROT"), no "Select primary S…".
6 Comparative placeholder + no Select-All Done (WP3) Empty comparative shows localized placeholder; ProjectSelector hideSelectAll prop.
7 Scope Selector Dropdown Variant N/A (already on HEAD via PR #2219) WP4 investigation confirmed it was already in.
8 View toggles as dropdown menu Done (WP5) Single eye-icon button opens DropdownMenu with two checkbox items (Hide Matches, Show Verse Text).
9 Performance Deferred (perf-profile.md captured in WP9; fixes in follow-up PR) Profile lives in ai-prompts; fix work scoped separately.
10 Equal column widths Done (WP8) All 7 project columns 107px wide; Reference 96px fixed.
11 Sticky header z-index Done (WP8) Table section wrapped in tw-relative tw-z-0 so the toolbar's z-10 wins over the thead's z-20.
12 Menu items (Copy/Print/Save/Inventory/Settings) Done (WP6) Verified: "Open Project Settings…", "Copy all text to clipboard", "Print Markers Checklist", "Save Markers Checklist", "Markers Inventory…", "Markers Checklist Settings". Print/Save fire NotificationService toasts saying "not yet implemented". Wordlist could not be reproduced in current build.
13 \rem \rem / \toc1 \toc1 duplication Done (WP1) Backend dropped the prepended TextItem in PostProcessParagraph. Visual: each marker appears once.
14 Bidirectional same-project filter Done (WP3) Primary popover shows only the 4 projects not currently in the comparative list (verified via list inspection).
15 Match-row coloring Done (WP2 — unit tests only) Implementation present: tw-bg-primary tw-text-primary-foreground on isMatch rows. Visual verification fell back to unit tests because real match data was unreachable in the GEN 2:23 fixture.
16 Clickable verse-number goto Done (WP2) <button data-testid="checklist-verse-goto"> wraps verse-number <sup>; onClick calls onGotoVerseClick(verseRef).
17 Settings dialog focus Done (WP2) useEffect + requestAnimationFrame focuses equivalentMarkersInputRef after the Radix focus trap activates. Code-verified; live verification difficult through CDP cross-iframe focus tracking.
18 Filter dropdown Space scroll N/A (already on HEAD via PR #2219's command.tsx Space-handler) WP4 investigation confirmed.
19 Character-style rendering Done (WP2) Any character-style item renders as <span className="usfm_{characterStyle}" data-character-style="{characterStyle}">; CSS catalog lives in checklist-usfm-styles.scss. Cells wrap content in checklist-formatted-font parent class.
20 ProjectSelector initial-Open-tabs Deferred — flag to ProjectSelector team Cross-feature, out of scope.
21 ProjectSelector list-refresh on install/open/close Deferred — flag to ProjectSelector team Cross-feature, out of scope.
22 Persist comparative projects, hideMatches, showVerseText, equivalent markers, marker filter Done (WP7) Hook useChecklistDefaults reads/writes user-setting platformScripture.markersChecklistDefaults. Verified live: previously-selected 7 comparative texts and dialog values (p/q, q1 q2) reappear on reopen.

Plan deviations

  • WP4 produced no commit. PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 (which landed before this branch was based) already contained the cherry-picked Dropdown Variant scope selector and the command.tsx Space-handler. Findings #7 and Prepare release #18 were satisfied on HEAD without additional work; WP4 investigation confirmed and documented this.
  • #15 visual proof fell back to unit tests. Match rows require the chapter / range to contain a row where every comparative agrees with the primary at the same paragraph marker. The exercised GEN 2:23 fixture had no such row; the implementation is covered by unit tests in checklist.component.test.tsx.
  • Fix ReadMe icon #12 Wordlist could not be reproduced in the current build — likely already removed since the 2026-05-05 review.

Known issues

  • WP6 Sonner toast z-index may be obscured in some layouts (e.g. floating webview). Reviewer feedback during testing would be appreciated.
  • Pre-existing storybook test failures in lib/platform-bible-react/ for DEFAULT_SCROLL_GROUP_LOCALIZED_STRINGS — these are unrelated to this PR and exist on ai/main.
  • Pre-existing lint warning for import/no-named-as-default on ProjectSelector in lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx — unrelated.

Tested

Live walkthrough against running app (CDP-attached headless Electron). Screenshots captured to /tmp/wp-final-*.png:

  • Opened Markers Checklist via PAPI platformScripture.openMarkersChecklist.
  • Verified single-toolbar layout, populated comparatives from persisted user setting (verifying #22 round-trip).
  • Walked the outer hamburger: all 6 expected items (Open Project Settings… / Copy all text to clipboard / Print Markers Checklist / Save Markers Checklist / Markers Inventory… / Markers Checklist Settings).
  • Opened primary popover: only the 4 projects not selected as comparatives appear (verifying #14).
  • Selected ROT as primary, table rendered with 8 equal-width project columns + smaller fixed Reference column.
  • Confirmed markers render once per paragraph (no \q1 \q1 duplication — Fix Lint warnings #13).
  • Eye-icon opened DropdownMenu with two checkbox items (#8).
  • Hovered ROT header — tooltip "rolf test" appeared, full-cell hover bg-accent/50 (Fix release #4).
  • Inspected DOM stacking context — table thead z-20 inside tw-relative tw-z-0 wrapper; toolbar z-10 wins (Add debugging to GHA workflows #11).
  • Opened Markers Checklist Settings dialog; Tab cycles inputs (Equivalent markers → Help button → Markers-to-display → Cancel → Save) inside the dialog only.
  • Toggled comparative-text count and view modes; cleared dialog inputs and saved — all persisted across reopens (#22).

Test plan

  • npm run lint — 0 errors (1 pre-existing warning, unrelated)
  • npm run typecheck — clean
  • npm test — TS suites pass except the 5 pre-existing storybook failures noted above
  • dotnet test in c-sharp-tests/ — 706 passed, 0 failed, 6 skipped
  • CI green
  • Tested by Rolf

🤖 Generated with Claude Code


This change is Reviewable

@rolfheij-sil
rolfheij-sil force-pushed the ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026 branch from 431d7b6 to 3693c42 Compare May 18, 2026 12:23
@rolfheij-sil
rolfheij-sil force-pushed the ai/feature/markers-checklist/followup-ux2-rolf-05-12-2026 branch from 11fc3e5 to 14868ea Compare May 18, 2026 13:00
@rolfheij-sil
rolfheij-sil force-pushed the ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026 branch from 3693c42 to 8f5a038 Compare May 26, 2026 11:45
…ect data type constants

For new MarkerNames PDP data types — see tj-review-design.md §2.1.
…HeadingMarkers + JoinedTextLanguage

New data types follow the canonical get/set/subscribe trio with throwing setters
(mirrors the Versification PDP pattern from PR #2277). The underlying stylesheet +
language data is read-only — owned by libpalaso, not writable through PDPs.
…rs on ParatextProjectDataProvider

Returns scParagraphStyle + scSection markers from the project's stylesheet,
per book. Read-only — Set throws. Used by TS-side checklist orchestrator
for INV-009 heading-paragraph classification.
…xtLanguage on ParatextProjectDataProvider

Returns scrText.GetJoinedText(bookNum).Settings.LanguageID.Id with fallback to
the project-level language tag. Per-book language is needed for sigla projects
that join multiple base-language texts per book.
…hods into PDP GetFunctions

Registers all four delegates (get + throwing set per data type) so the TS
side can invoke them over the wire. Mirrors the Versification PDP wiring
pattern from PR #2277.
… checklist marker settings

Stubs accept any string until Phase 3's parseMarkerSettings lands (Task 3.1
will replace the equivalent-markers validator with the real strict parse).

Also adds the two new keys to the ProjectSettingTypes augmentation in
platform-scripture.d.ts — required so ProjectSettingValidator<...> typechecks
for the new setting names. The plan's escalation note (Phase 2 Task 2.3, "When
to escalate") explicitly anticipated this; without the augmentation the
generic-type lookup fails with TS2344. Declarations are minimal — just the key
plus a one-paragraph TSDoc — matching the existing valid/invalidMarkers shape.
…rSettingsValidationTests)

Pure TS strict parser; replaces the backend validateMarkerSettings NetworkObject method.
Used by both the Marker Settings dialog's keystroke feedback (live) and the new
platformScripture.checklistEquivalentMarkers project-setting validator (set-time).
24 tests ported byte-for-byte from MarkerSettingsValidationTests.cs.
initializeMarkerMappings is the lenient runtime parser (VAL-005 — silently skips
invalid tokens). hasSameValue implements INV-002 / BHV-104 / INV-005 (bidirectional
marker equivalence). Test cases ported from MarkersDataSourceTests.cs.
…DataSource.PostProcessParagraph)

INV-004 (prepend backslash-marker TextItem) + BHV-103 (drop body items when showVerseText=false).
RTL prefix is applied by the upstream walker on original text items, not by this module — the
synthetic \\marker label is never RTL-prefixed, matching C# MarkersDataSource.PostProcessParagraph.
Pure function — never mutates input.
…tRowBuilder.BuildRowsMergingCells)

Simple column-zip aligner. Per working-docs/tj-review-consumer-inventory.md § 6, verse-bridge
expansion and MaxCellsToGrab capping are NOT implemented in this module — they happen upstream in
buildChecklistData (Phase 5) by pre-grouping per-verseRef cells. This module just lines up cells
across columns by their reference and emits empty placeholders where a column lacks coverage.
…nt for C# GetTokensForBook)

Pure function over USJ. Replaces the C# state-machine token walker with a straight iteration of
top-level usj.content nodes (paragraphs, chapter, verse markers). Notes/figures are skipped
implicitly — they're nested, not top-level. Implements INV-009 heading forward-scan +
FB-35863 chapter-boundary stop.

Char-marker handling drops the plan's \\ior LinkItem special-case per consumer-inventory § 3:
all char nodes flatten to a TextItem with characterStyle: marker, matching the C# walker at
ChecklistService.cs ~1082-1095. LinkItem is reserved for the CrossReferences branch (out of
scope for the Markers checklist).

Walker emits WalkerParagraph[] (the walker-internal shape with verseRefStart + isHeading); the
orchestrator strips those fields before populating the wire ChecklistParagraph[].

19 unit tests cover: basic walking, user markerFilter, heading classification, INV-009 forward
scan (including consecutive headings + FB-35863 chapter boundary), nested-note skipping, item
extraction (text/verse/char), verseRef tracking state (intro=0, inherit-last-verse, chapter
reset), and Canon book-id mapping.
…PDP helper)

Acquires the 3 PDPs (USJ_Book, MarkerNames, base) and fetches everything in parallel.
Single call site in the orchestrator; easy to test in isolation. Introduces PapiLike
structural interface so the helper avoids depending on @papi/frontend (React-bound,
not importable from pure-Node test runs).
…ChecklistService.BuildChecklistData)

Composes USJ walker + post-processor + per-verseRef cell grouping (§6) + row builder
+ match detection + hideMatches filter + EditLinkItem placement (BHV-114) + empty-result
variant detection (§7.2 — markerFilter.size===0 gate only). Replaces the entire backend
ChecklistService.BuildChecklistData pipeline.

Highlights:
- Per-verseRef paragraph grouping happens here (scenario-01 row 0 = 24-paragraph intro cell).
- markerFilter strips backslashes before tokenizing (§7.1 / VAL-001).
- Both platform.name (header) and platform.fullName (tooltip) resolved per column (§7.3).
- EditLink gated per-cell on column-level isEditable AND non-undefined cell.reference,
  appended to last paragraph (mirrors c-sharp/Checklists/ChecklistService.cs:565).
- row.includeEditLink populated post-row-build per VAL-007 cond 2 row-level signal.

20 new tests cover end-to-end behaviors: single-column shape, verseRef grouping,
INV-002 single-column match override, multi-column match/no-match, hideMatches +
excludedCount, empty-result variants (identical vs noResults), markerFilter parsing
+ backslash strip, ProjectNotFoundError on missing project, column headers, booksPresent
intersection.
… + project settings

- buildChecklistData now invoked locally (TS); no network-object roundtrip
  (addresses TJ C3).
- Marker settings storage: useWebViewState → useProjectSetting (cross-web-view
  persistence on the same project; addresses TJ C3a).
- validateMarkerSettings call replaced with local parseMarkerSettings (no
  per-keystroke PAPI roundtrip; addresses TJ C3b). The validator adapter
  pre-localizes the parser's errorMessageKey before passing it to the dialog
  (consumer-inventory § 4, Case A).
- isEditable: orchestrator reads platform.isEditable per-(column, book) via
  the PDP; the previous useChecklistService hook is no longer needed.
- MarkerSettingsValidationResult inlined into marker-settings-dialog.component
  so the dialog is self-contained (the public contract type goes away in 6.3).
…t Types region

The backend ChecklistNetworkObject is gone; no consumer should see these types
on the contract surface. TS-local types live in
extensions/src/platform-scripture/src/checklists/checklist.types.ts.
…klistNetworkObject registration

~2600 lines of C# removed. Aggregation now lives entirely in TS
(extensions/src/platform-scripture/src/checklists/). See tj-review-design.md §2.6.
Tests the deleted network-object wire methods. Web-view-level E2E suites
(functional-UI-PKG-002, functional-UI-PKG-003, journey) cover behavior.
rolfheij-sil and others added 21 commits May 26, 2026 22:21
…ws on same project

Validates the new project-scoped persistence introduced by Change B (settings
moved from useWebViewState to useProjectSetting). See tj-review-design.md §R4.
…rection (not platformScripture.textDirection)

The orchestrator was reading a setting key that doesn't exist on PT10's PDP
contract. The canonical platform-base text-direction setting is
'platform.textDirection' (used elsewhere in checklist.web-view.tsx and the
component-side types). Post-Phase-6 app smoke surfaced the error:

  ChecklistWebView: buildChecklistData failed: JSON-RPC Request error
  (-32000): Could not find project setting platformScripture.textDirection.

This commit aligns fetch-column-book-data + its test fixtures + the
build-checklist-data test fixtures with the canonical key.
…7 baseline parity

Five fixes to make the TS orchestrator behave correctly against live PDPs:

1. USJ_Book PDP takes SerializedVerseRef (not bookNum). The existing
   platformScripture.USJ_Book.getBookUSJ method signature expects
   {book, chapterNum, verseNum}; passing a bare number serialized to an
   incomplete VerseRef that the backend rejected (-32602 deserialization
   error). Aligns with the existing consumer pattern in
   search-results-in-book.component.tsx:60-66 ({book, chapterNum:1, verseNum:0}).

2. VAL-003 applies to ANY book (not just GEN). Original guard checked
   ref.book === 'GEN' && chapterNum === 1 && verseNum === 1 — that's
   too narrow. C# applies the verse-0-inclusion at chapter-1-verse-1
   regardless of book; scenario-10 (MAT 1:1–1:25) baseline depends on
   it (the MAT 1:0 intro material row must be included).

3. Walker output filtered to verseRange. The USJ walker emits every
   paragraph in the book; the orchestrator's verseRange MUST gate which
   paragraphs make it into cells. Without this, a single-chapter request
   produced 571 rows (whole-book) instead of 8 (chapter 1).

4. ProjectNotFoundError no longer extends Error. SWC's class-extending-
   Error polyfill touches Function.bind.apply(...), which the extension-
   host sandbox blocks → "ReferenceError: Function is not defined" at
   extension activation. Replaced with a factory + type-guard pattern:
   makeProjectNotFoundError(id) and isProjectNotFoundError(err). The
   factory attaches a sentinel name + code to a plain Error instance.

5. checklist.web-view.tsx updated to use isProjectNotFoundError instead
   of `instanceof ProjectNotFoundError` (which would no longer narrow
   with the factory pattern).

After these fixes, 9 of 10 baseline scenarios pass structural fingerprint
parity. Scenario 02 (whole-book single-column) has +9 rows (571 vs 562)
due to verse-bridge / MaxCellsToGrab merge logic explicitly out-of-scope
per consumer-inventory § 6 — separate follow-up if needed.
…nd + lint cleanup

- Removes platformScripture.markersChecklist.buildForVerification command and
  its CommandHandlers augmentation. The verification harness was used to diff
  the TS orchestrator output against the captured C# baseline (Task 7.1); now
  that 9/10 scenarios pass parity and the divergence is documented, the
  command is no longer needed and must not ship.
- Refactors makeProjectNotFoundError + isProjectNotFoundError to avoid the
  no-type-assertion lint error (Object.assign returns the literal-narrowed
  type; type guard uses TS 4.9+ 'in' narrowing on Error's unknown 'code').

Phase 7 verification record is in
.context/features/markers-checklist/working-docs/tj-review-consumer-inventory.md.
- #13: PostProcessParagraph no longer prepends `\marker` as a TextItem.
  The UI renders the marker from paragraph.Marker. INV-004 revised:
  Items contains only the original verse-text items (or empty when
  showVerseText=false).

- #3 backend: PostProcessRows gains hasComparativeTexts parameter. The
  "Comparative texts have identical markers" Identical variant is now
  only emitted when at least one comparative is configured. The empty-
  with-no-comparatives case returns NoResults instead.

- #3 frontend: empty-results fallback no longer defaults to the
  identical-markers string. New keys
  %markersChecklist_emptyResult_noResults% / _selectProject% drive a
  context-appropriate message.

Co-Authored-By: Claude Code <noreply@anthropic.com>
… deferred)

- #19: character-style items now render as <span className="usfm_{marker}">
  with styling from a new checklist-usfm-styles.scss partial mirroring
  scripture-editor's _usj-nodes.scss character-style subset. The SCSS is
  imported via ?inline in checklist.web-view-provider.ts and concatenated
  with the tailwind styles passed to PAPI (the extension's webpack config
  uses sass-loader without style-loader, so the styles must be threaded
  through the web-view-provider's styles field rather than the standard
  side-effect import). No more literal "(\\nd Lord)".
- #15: match rows get tw-bg-primary tw-text-primary-foreground on both
  the reference and project cells when row.isMatch === true.
- #16: verse-number items in cells are now click-to-navigate buttons
  routed through onGotoLinkClick. The button's verseRef is reconstructed
  as `{book chapter}:{verseNumber}` from the owning cell's reference.
  Plain-superscript fallback preserved for read-only contexts.
- #17: settings dialog focuses the equivalent-markers input on each open
  transition via requestAnimationFrame so Radix's focus trap is active
  before we focus. Replaces the autoFocus prop on the Input which fired
  before the focus trap mounted.
- #18: deferred to be re-verified after WP4 cherry-picks
  shadcn-ui/command.tsx (which addresses the underlying cmdk Space
  propagation). Reopen if filter-dropdown space still scrolls the table
  after WP4.

Adds Vitest regression tests for #19 (character-style className), #15
(match-row coloring on the reference cell), and #16 (verse-goto button
wiring + verseRef reconstruction). Per CLAUDE.md the test file uses the
`// @vitest-environment jsdom` directive because the extensions vitest
config defaults to environment: 'node' and the new tests need RTL.

Co-Authored-By: Claude Code <noreply@anthropic.com>
- #1: dropped the inner ChecklistTool TabToolbar. Selectors + view
  toggles now render inline in a single sticky row. The outer Platform
  tab chrome hamburger will host our menu items via WebViewMenu
  contributions (see WP6); the now-dead `projectMenuData` /
  `onSelectProjectMenuItem` props + clipboard/menu wiring have been
  removed to keep the web-view free of orphaned code.
- #5: primary-project trigger no longer shows the truncating
  "Select primary S..." placeholder. The trigger renders the project's
  short name; the localized hint remains available via the aria-label /
  tooltip. Also dropped the redundant `primaryProjectName` lookup that
  was only used to populate the now-defunct placeholder.
- #6: comparative-texts trigger shows a proper placeholder
  ("Select comparative texts") when empty. Added an opt-in
  `hideSelectAll` prop to ProjectSelector's `project-multi` mode and
  set it on the comparative trigger — selecting every project as a
  comparative is rarely useful and creates accidental wide queries.
- #14: bidirectional same-project filter. Primary list hides every
  selected comparative; comparative list hides the primary. Filter
  logic extracted to `checklist-project-filter.utils.ts` with 7
  unit tests so the rule can't regress silently.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Replace the ToggleGroup of Eye/EyeOff + Book/BookOpen buttons with a
single eye-icon DropdownMenu containing checkbox items:
- Hide matches (disabled when columnCount <= 1)
- Show verse text

Per UX-2 finding #8: the icon does not change with state; the
checkbox marks inside the menu show the current state.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…toasts (#12)

Hamburger menu rework per UX-2 finding #12:

- Rename Copy -> "Copy all text to clipboard"; show Sonner success toast
  ("Checklist text copied to clipboard.") on successful clipboard write
- Rename Settings -> "Markers Checklist Settings"; sync the dialog
  title (`markersChecklist_settings_title`) so menu + dialog match
- Add "Print Markers Checklist" -- handler fires "not yet implemented"
  toast via NotificationService (`papi.notifications.send`)
- Add "Save Markers Checklist" -- same not-implemented toast pattern
- Add "Markers Inventory..." -- opens the existing
  `platformScripture.markersInventory` web view via the already-
  registered `platformScripture.openMarkersInventory` command

Wiring details:

The plan's original "intercept Copy in the web view" path no longer
applies because WP3 deleted the inner TabToolbar. Menu items now
dispatch through the outer Platform.Bible tab chrome, which calls
`papi.commands.sendCommand(command, tabId)` -- so the web view never
receives a direct `onSelectProjectMenuItem` callback. To preserve
live `visibleData` access for the clipboard copy we introduce a
companion broadcast network event (`CHECKLIST_COPY_REQUEST_EVENT`,
mirroring the existing `CHECKLIST_OPEN_SETTINGS_EVENT`): the
`copyMarkersChecklist` command in main.ts emits the event, and the
web view subscribes via `useEvent` to build the clipboard text from
the current `visibleData` snapshot.

Clipboard write resilience: when the outer-chrome menu fires, focus
sits on the main-frame menu, not the web view's iframe -- so the
async clipboard API throws "Document is not focused". The handler
calls `window.focus()` first and falls back to a hidden-textarea
`execCommand('copy')` if the async path is still unreachable.

All "not implemented" / "copy succeeded" messages use
`papi.notifications.send({severity:'info'})` which surfaces as a
non-blocking Sonner toast in the renderer's mounted <Toaster />.

Verification: visual-verified via CDP -- menu order matches plan,
all toasts render, clipboard receives table content, Markers
Inventory opens, Settings dialog title updated. Lint clean, typecheck
clean, 136/136 platform-scripture vitest tests pass, 706/706 C# tests
pass.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Persist comparative projects, view toggles, equivalent markers, and
marker filter to a user setting so a new checklist tab — or a fresh
app session — inherits the user's last-committed defaults.

- New setting: platformScripture.markersChecklistDefaults
  (declared in contributions/settings.json + SettingTypes augmentation
  in platform-scripture.d.ts).
- New hook: useChecklistDefaults reads the setting on mount and
  exposes a writeDefaults callback that merges partial updates with
  the latest snapshot before calling papi.settings.set. Errors are
  swallowed + logged so a failed persist degrades to per-tab-only
  persistence.
- Split checklist.web-view's body into an outer ChecklistWebView
  (reads the setting, gates rendering on isLoading) and an inner
  ChecklistContent (runs all the useWebViewState slots). The gate is
  required because useWebViewState's `useState(() => ...)` lazy init
  consults the default exactly once — without the gate, the first
  render would seed every slot with the static defaults and silently
  ignore the persisted value.
- A new useEffect in ChecklistContent mirrors the persisted slots
  back to the setting on every change (last-write-wins for cross-tab
  races, accepted as a minor UX wart).
- Scope and verse range are NOT persisted (matches PT9 memento
  behaviour and reviewer instruction).

Cross-restart visual verification confirmed: comparative project +
equivalentMarkers + markerFilter all survive `./.erb/scripts/refresh.sh`
and seed a freshly-opened Markers Checklist tab.

Tests: 6 new vitest cases in use-checklist-defaults.test.ts pin the
hook contract (loading state, persisted-value exposure, partial-merge,
read-fail fallback, write-merge, write-fail swallow). 173/173 platform-
scripture tests pass; npm run typecheck + npm run lint clean.

Co-Authored-By: Claude Code <noreply@anthropic.com>
- #10: enforce equal column widths via `table-fixed` layout +
  `tw-w-24` on the first `<th>` (Ref column). TanStack's DataTable
  does not honor `ColumnDef.size`, and inline widths on inner divs
  do not propagate to the `<th>` under table-fixed — widths must
  live on the `<th>` itself. Applied via Tailwind arbitrary
  variants on the `<section>` wrapper, no shared-lib changes.
  Replaces the previous auto-layout sizing where the widest cell
  drove the column width (making columns unequal).
- #11: wrap the table section in `tw-relative tw-z-0` so the sticky
  thead's intrinsic `z-20` (set by shadcn-ui Table) stays inside a
  new stacking context and never out-stacks the toolbar (`z-10`).
- #4: header tooltip + hover now cover the entire header cell
  (was just the text span). Cursor stays as the default arrow
  (neither pointer nor text). A subtle `bg-accent/50` on hover
  makes the hoverable area discoverable.

Co-Authored-By: Claude Code <noreply@anthropic.com>
WP2's `tw-bg-primary tw-text-primary-foreground` was unreadable in both
light and dark modes — inner spans hardcode their own text colors
(`tw-text-foreground`, `tw-text-primary`, `tw-text-muted-foreground`),
overriding the outer container's `tw-text-primary-foreground`. Net
effect: same color on same color = invisible content. Additionally the
bg-color sat on the inner content div, so the colored region didn't
cover the full table cell (an unstyled border framed every match row).

Fix: replace with `tw-bg-primary/30` (subtle tint) and bleed the tint
to the cell edges via `-tw-m-4 tw-px-6 tw-py-6`, which cancels
`TableCell`'s built-in `tw-p-4` and restores the content offset. The
30% opacity lets the inner spans' default colors stay readable in
both light AND dark modes.

Approach A (negative-margin bleed on the cell's outermost element)
was chosen over modifying the shared `DataTable` to honor a
`columnDef.meta.className` (Approach B) because the shared DataTable
renders `<TableCell>{flexRender(cell, ctx)}</TableCell>` with no hook
for per-cell `<td>` classes — adding one would expand the PR scope to
`lib/platform-bible-react` and require ADR coverage.

- Drop `tw-text-primary-foreground` (no longer needed)
- Replace inner-div bg with outer-element bg that fills the full cell
- Update RTL tests to assert `tw-bg-primary/30` and the absence of
  `tw-text-primary-foreground`

Co-Authored-By: Claude Code <noreply@anthropic.com>
…(#15)

The first fix attempt (3b8b99b) tried to bleed a tw-bg-primary/30 inner
container past TableCell's tw-p-4 with negative margins. Rolf reported the
colored region still only covered the text/content area, not the full cell
rectangle.

This iteration applies the tint at the row level via a new getRowClassName
prop on the shared DataTable component. The row's `<tr>` background paints
under every `<td>`, so each cell truly fills edge-to-edge. Color is
centralized in a single MATCH_ROW_BG_CLASS module-scope constant for
one-place tuning.

Changes:
- lib/platform-bible-react/src/components/advanced/data-table:
  Add optional getRowClassName?: (row) => string | undefined prop. Forwarded
  to `<TableRow>`'s className. No behavior change when the prop is omitted.
  Dist rebuilt.
- extensions/src/platform-scripture/src/components/checklist.component.tsx:
  MATCH_ROW_BG_CLASS = 'tw-bg-primary/20' (down from /30 — Rolf: too loud).
  Pass via getRowClassName when row.original.isMatch. Removed the negative-
  margin construction from both refColumn and projectColumns cells.
- checklist.component.test.tsx: RTL tests now walk up from the reference
  cell to the `<tr>` and assert the bg class lives there, not on inner
  content.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Disable the outer Platform.Bible tab chrome for the markers checklist
(shouldShowToolbar=false) and host the hamburger menu directly on the
inner TabToolbar — partial reversal of WP3's "drop the inner toolbar"
decision, prompted by Rolf's request to consolidate everything on one
toolbar instead of duplicating chrome.

Changes:

- checklist.web-view-provider.ts: shouldShowToolbar false.
- checklist.types.ts: restore the projectMenuData + onSelectProjectMenuItem
  props on ChecklistToolProps (with Localized + MultiColumnMenu imports).
- checklist.component.tsx: restore the TabToolbar import and use it again
  as the toolbar wrapper, passing the project-menu props through. Wrap
  renderToolbarStart and renderToolbarEnd in `tw-flex tw-h-full
  tw-items-center` divs so selectors / view-menu trigger align
  horizontally with the hamburger (TabToolbar's inner area divs hardcode
  items-start, which top-aligned them while the hamburger center-aligned).
- checklist.web-view.tsx: restore MARKERS_CHECKLIST_WEB_VIEW_TYPE +
  DEFAULT_WEBVIEW_MENU constants, the useData(papi.menuData...).WebViewMenu
  fetch + memoized narrowing, and the handleSelectProjectMenuItem dispatch
  (papi.commands.sendCommand for every item — no local Copy intercept,
  reuses WP6's CHECKLIST_COPY_REQUEST_EVENT plumbing unchanged).

Menu items (Copy / Print / Save / Markers Inventory / Settings) still
flow through the same registered commands in main.ts; only the dispatch
origin moves from outer chrome to inner toolbar.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…th PR #2212

The earlier WP4 investigation concluded the Dropdown Variant was already on
HEAD via PR #2219; that conclusion was incomplete. The shared component's
JSX is present on HEAD, but four call-site / shared-lib gaps kept the
markers-checklist's scope dropdown from matching the design in PR #2212's
Dropdown Variant story:

1. The Navigate footer was never rendering — `checklist.web-view.tsx` passed
   `currentScrRef` but not `onCurrentScrRefChange`, and the component gates
   the footer on the callback. Fixed by wiring `onCurrentScrRefChange=
   {setLiveScrRef}` (option A — broadcasts via the scroll group; does NOT
   raise the editor tab, to keep focus inside the checklist).

2. Footer label read "Navigate" instead of "Change current reference".
   `localizedStrings.json` contributed "Navigate" for
   `webView_scope_selector_navigate`; renamed to "Change current reference"
   to match the design.

3. "Choose specific books" was missing from the dropdown because
   `availableScopes` excluded `selectedBooks`. Added the scope (option C
   from triage); `handleScopeChange` intercepts `selectedBooks` and fires a
   `papi.notifications.send` toast — "Choose specific books is not yet
   supported for the markers checklist." — without committing the scope
   change, so the previous scope stays active. The backend's
   ChecklistRequest.verseRange contract only supports contiguous start/end
   ranges, so a real implementation needs a separate follow-up.

4. Verse / Chapter / Book dropdown items weren't showing their scrRef
   suffixes (`MAT 5:3`, `MAT 5`, `MAT`). Root cause was in the shared
   ScopeSelector: `DropdownMenuContent`'s `tw-min-w-[12rem]` (192px) sat
   below the `DROPDOWN_NARROW_THRESHOLD_PX` (200), so `isDropdownNarrow`
   stayed true and suffixes were hidden. Bumped to `tw-min-w-[14rem]`
   (224px) so the content rect lands at ~216px (after shadcn's `tw-p-1`),
   clearing the threshold. Rebuilt platform-bible-react dist. Affects every
   ScopeSelector dropdown-variant consumer — 32px wider minimum, low risk.

Files:
- extensions/src/platform-scripture/src/checklist.web-view.tsx
- extensions/src/platform-scripture/contributions/localizedStrings.json
- lib/platform-bible-react/src/components/advanced/scope-selector/scope-selector.component.tsx
- lib/platform-bible-react/dist/* (rebuilt)

Co-Authored-By: Claude Code <noreply@anthropic.com>
…essParagraph (port UX-2 #13)

WP1 (commit c090ba6) revised the C# `MarkersDataSource.PostProcessParagraph`
to stop prepending `\marker` as a synthetic TextItem at items[0]: the UI now
renders the marker label from `paragraph.marker` separately
(`checklist.component.tsx` `renderParagraphLine`), so the synthetic TextItem
caused a duplicate label.

PR #2276 deleted the C# checklist orchestrator (replaced by the TS orchestrator
introduced in PR #2277), but the TS port of `postProcessParagraph` still carried
the pre-WP1 behavior. Port the WP1 semantic intent here:

- `showVerseText=true` → pass through `paragraph.items` unchanged.
- `showVerseText=false` → return empty items list.

The UI's `\${paragraph.marker}` label remains the single source of the marker
rendering.

Tests updated:
- TS-009 / TS-010 / TS-067 now assert the post-WP1 contract (no synthetic
  TextItem, empty items when showVerseText=false).
- VerseItem-preservation tests adjusted: showVerseText=true preserves the
  walker's items as-is; showVerseText=false returns empty.
- RTL test simplified: postProcessParagraph never touches text, so the
  walker's RTL-prefixed item passes through unchanged.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…tical variant (port UX-2 #3 backend)

WP1 (commit c090ba6) added a `hasComparativeTexts` parameter to the C#
`PostProcessRows` so the "Comparative texts have identical markers" variant is
only emitted when at least one comparative is configured. The empty-with-
no-comparatives case falls through to `noResults`, which the React layer maps
to a generic "no markers found" string via the
`%markersChecklist_emptyResult_noResults%` key added in the same WP1 commit.

PR #2276 deleted the C# orchestrator, so the WP1 backend change applied only to
surviving files (the React component + the localized-strings registry). The TS
orchestrator added in PR #2277 still emitted `identical` on single-column empty
results, which made the React fallback chain surface "Comparative texts have
identical markers." on first load with no project selected.

Port the WP1 semantic intent:
- `buildEmptyResultMessage` now takes a `hasComparativeTexts: boolean` arg.
- The `identical` variant is gated on BOTH `markerFilter.size === 0` AND
  `hasComparativeTexts === true`.
- All other empty-result cases (non-empty filter OR no comparatives) emit
  `noResults`, carrying the searched markers + books for the localized
  template substitution.

Tests updated:
- The single-column "empty filter → identical" test now expects `noResults`
  (this is the case UX-2 #3 specifically fixed).
- The booksPresent-skipping test (single-column, empty filter) also flips to
  `noResults` for the same reason.
- New regression test: two-column + empty USJ + empty filter → `identical`
  (preserves the original semantic when comparatives ARE present).

Co-Authored-By: Claude Code <noreply@anthropic.com>
…d is undefined

Port of pre-existing C# fix 315b59c (whose target was deleted in PR #2276
along with the rest of the C# orchestrator) into the TS orchestrator added in
PR #2277.

Per the `ScriptureRange` contract (mirrors PT9's
`ChecklistService.ResolveVerseRange`):
- `verseRange === undefined` → entire project (GEN 1:0 → REV 22:21).
- `verseRange.start` defined, `verseRange.end` undefined → single-verse range
  at `start` (collapse `end` to `start`).
- `verseRange.end` defined → use both ends as-supplied.

The TS orchestrator previously had:
  const endRef = request.verseRange?.end ?? defaultEndRef();
which fell back to REV 22:21 when only the start was supplied, expanding a
single-verse request into a whole-project scan.

VAL-003 (GEN 1:1 → GEN 1:0) is still applied to the start, but only to the
start — the end is left as-supplied so a single-verse request at GEN 1:1 still
resolves to GEN 1:1 on the end.

Test: new `verseRange resolution (ScriptureRange contract)` describe with two
cases:
- end undefined → only the single requested verse is in the result.
- end defined → both ends are honored as-is.

Co-Authored-By: Claude Code <noreply@anthropic.com>
… rebase

WP3 (the rebase step) took the incoming dist files for the conflict-resolved
state, but subsequent UX commits modified the lib source (radix-ui aggregated
import in popover.tsx, LinkedScrRefButton @experimental marker, ScopeSelector
dropdown variant, data-table.component.tsx). The dist artifacts were therefore
out of sync with the source.

Regenerated via `npm run build` in lib/platform-bible-react. The bundle is
considerably smaller (10k → ~12k lines) because the rebased source now uses
the unified `radix-ui` package import instead of individual `@radix-ui/*`
sub-packages, which de-duplicates radix internals in the bundle.

No source-level changes — only dist artifacts.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…prefix

The rebased UX-2 commits were authored against Tailwind v3 (`tw-` hyphen
prefix). `ai/main` has since converted to Tailwind v4 (`tw:` colon prefix),
which makes all v3 prefix classes silently invalid.

106 v3 occurrences across 5 source files. Most consequential:
`[&_table]:tw-table-fixed [&_table]:tw-w-full [&_thead_th:first-child]:tw-w-24`
on the checklist's table wrapper — these were intended to give each project
column equal width (UX-2 finding #10). Without them, the table fell back to
`table-auto`, sized each column to its widest content, and pushed projects
off-screen when verse text was shown.

Two mechanical transformations:
- `[arbitrary]:tw-utility` -> `tw:[arbitrary]:utility` (arbitrary-variant case)
- `tw-utility` -> `tw:utility` (standalone)

A third manual pass fixed variant-misorder cases produced by the second pass
where the input already had a variant prefix (e.g. `hover:tw-underline` ->
`hover:tw:underline` is invalid v4 syntax; the correct form is
`tw:hover:underline`). Affected `hover:`, `focus-visible:`, and
`data-[highlighted]:` variants.

Rebuilt `lib/platform-bible-react/dist` to pick up source-level conversions in
project-selector + scope-selector. Visual-verified equal-column rendering with
TPTS + NBV21 + RH2 + showVerseText=true (screenshot: /tmp/ux2-fix-equal-columns.png).
The shadcn TableCell component applies tw:whitespace-nowrap to every <td>
by default. Combined with the table-fixed enforcement, this meant body
cells had fixed widths but content rendered on a single line — so long
verse text (when showVerseText is on) overflowed each cell and visually
overlapped neighbouring columns.

Two surgical Tailwind arbitrary-variant rules added to the table wrapper:

  - tw:[&_td]:whitespace-normal overrides shadcn's stock nowrap for body
    cells, so verse text wraps at word boundaries inside its column.
    <th> headers keep nowrap so project names like "TPTS" / "NBV21"
    don't break.
  - tw:[&_td]:align-top keeps body content top-aligned across cells of
    varying heights. Without this, a row with one short-marker cell and
    one long-paragraph cell would vertical-center the short content,
    which reads poorly when scanning.

The ref column body cells still render their refs on one line because
the LinkedScrRefButton (a shadcn Button) carries its own whitespace-nowrap
internally, narrower than the cell's 96px width.

Visual-verified with TPTS + NBV21 + RH2 (3 columns) + showVerseText=true:
text wraps within each column, no overlap.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@rolfheij-sil

Copy link
Copy Markdown
Contributor Author

ai/main was rebased onto main on 2026-06-12.

If this branch was built on the previous ai/main, it inherited commits whose SHAs were rewritten. To sync cleanly:

/rebase-onto-ai-main

from a root or workspace checkout. The command auto-detects the right rebase tag (ai/main-pre-rebase-2026-06-11-214839Z / ai/main-post-rebase-2026-06-11-214839Z) and uses git rebase --onto so only your unique commits get replayed.

See .claude/commands/rebase-onto-ai-main.md for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant