[P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages)#2278
Closed
rolfheij-sil wants to merge 43 commits into
Conversation
rolfheij-sil
requested review from
irahopkinson,
jolierabideau,
lyonsil and
tjcouch-sil
as code owners
May 15, 2026 08:47
6 tasks
rolfheij-sil
force-pushed
the
ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026
branch
from
May 18, 2026 12:23
431d7b6 to
3693c42
Compare
rolfheij-sil
force-pushed
the
ai/feature/markers-checklist/followup-ux2-rolf-05-12-2026
branch
from
May 18, 2026 13:00
11fc3e5 to
14868ea
Compare
rolfheij-sil
force-pushed
the
ai/feature/markers-checklist/followup-tj-review-rolf-05-11-2026
branch
from
May 26, 2026 11:45
3693c42 to
8f5a038
Compare
…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.
…stMarkerFilter project settings
… 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.
…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>
Contributor
Author
|
If this branch was built on the previous from a root or workspace checkout. The command auto-detects the right rebase tag ( See |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
fdf9f176f66068e929058aa80f398fb686a2a81f07668b5acf91ce56a97be6d686f5b0Per-finding status (Sebastian's 22)
count("[aria-label='Project']") == 1.comparativeTextIds.Count > 0; UI now shows "Select a project to view its paragraph markers."tw-w-full tw-cursor-default hover:tw-bg-accent/50div wraps the cell; tooltip "rolf test" verified on hover.hideSelectAllprop.tw-relative tw-z-0so the toolbar's z-10 wins over the thead's z-20.\rem \rem/\toc1 \toc1duplicationPostProcessParagraph. Visual: each marker appears once.tw-bg-primary tw-text-primary-foregroundonisMatchrows. Visual verification fell back to unit tests because real match data was unreachable in the GEN 2:23 fixture.<button data-testid="checklist-verse-goto">wraps verse-number<sup>;onClickcallsonGotoVerseClick(verseRef).useEffect+requestAnimationFramefocusesequivalentMarkersInputRefafter the Radix focus trap activates. Code-verified; live verification difficult through CDP cross-iframe focus tracking.command.tsxSpace-handler)<span className="usfm_{characterStyle}" data-character-style="{characterStyle}">; CSS catalog lives inchecklist-usfm-styles.scss. Cells wrap content inchecklist-formatted-fontparent class.useChecklistDefaultsreads/writes user-settingplatformScripture.markersChecklistDefaults. Verified live: previously-selected 7 comparative texts and dialog values (p/q,q1 q2) reappear on reopen.Plan deviations
command.tsxSpace-handler. Findings #7 and Prepare release #18 were satisfied on HEAD without additional work; WP4 investigation confirmed and documented this.checklist.component.test.tsx.Known issues
lib/platform-bible-react/forDEFAULT_SCROLL_GROUP_LOCALIZED_STRINGS— these are unrelated to this PR and exist onai/main.import/no-named-as-defaultonProjectSelectorinlib/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:platformScripture.openMarkersChecklist.\q1 \q1duplication — Fix Lint warnings #13).tw-relative tw-z-0wrapper; toolbar z-10 wins (Add debugging to GHA workflows #11).Test plan
npm run lint— 0 errors (1 pre-existing warning, unrelated)npm run typecheck— cleannpm test— TS suites pass except the 5 pre-existing storybook failures noted abovedotnet testinc-sharp-tests/— 706 passed, 0 failed, 6 skipped🤖 Generated with Claude Code
This change is