PT-4036/PT-4037: versification-aware synchronized scrolling

[lyonsil]

Summary

Implements Row 4 of PT-4031 ("Windows scroll together, versification-aware"), subtasks PT-4036 and PT-4037.

When windows in a scroll group span projects with different versifications, a shared book/chapter/verse previously landed each window on the same numbers but, at divergence points (Psalm headings, 3 John, deuterocanon, Greek/Hebrew numbering), a different actual verse. This adds versification-aware conversion so each consumer displays the corresponding verse in its own project's versification — porting the PT9 pattern.

Changes

• PT-4036 — conversion command (C#). New project-agnostic PAPI Individual Command platformScripture.mapVerseRefBetweenProjects(verseRef, sourceProjectId, targetProjectId) (c-sharp/Projects/VersificationConversionService.cs, registered in Program.cs), wrapping the existing VerseRef.ChangeVersification / ChangeVersificationWithRanges. null source ⇒ canonical English. Marked experimental via the registration documentation arg. Declared TS-side in CommandHandlers.
• PT-4037 — apply conversion in sync. The scroll group records the source projectId of its current ref (scroll-group.service-model.ts / .service-host.ts, session-only). useScrollGroupScrRef gains a projectId and converts the followed ref into the consumer's versification via the command (display-only, pass-through-safe, stale-guarded). The three internal callers pass their projectId (web-view wrapper, web-view host, toolbar via currentProject); the 11 extension consumers are unchanged.

Design note

The conversion is hosted as an Individual Command (not a per-project data provider): it takes both project IDs and is project-agnostic, so hosting it per-project would force every consumer to acquire its own project's Versification PDP — which hangs for projects that don't implement that interface. A command needs no acquisition. Per-project versification lookups (getFinalVerseNumber, etc.) correctly stay on the platformScripture.Versification interface.

Testing / verification

• npm run typecheck, lint (changed files), npm test (all suites, 0 failed incl. storybook), dotnet test (1294 passed / 0 failed / 6 skipped) — all green.
• New C# tests (real divergence, null-source-English, segment preserved, HasMultiple/bridge branch) and new TS host + hook tests (conversion + pass-through branches).
• Runtime, live app: the command converts PSA 51:1 (English) → PSA 51:3 (Original) with real .vrs data; in the UI, two projects in one scroll group sync to Psalm 51 with the toolbar showing the English-converted verse (51:1) while the group holds the Original verse (51:3) — cross-versification conversion confirmed end-to-end.

Known follow-up (separate, not this slice)

Navigating a scroll group to a book a consumer's project doesn't contain can trigger a book-corrupting write-back from the scripture editor's own (pre-existing) missing-book handling — observed only in a confounded multi-consumer state, independent of this conversion (the command preserves the book; reproduces with no conversion running). This is the separate editor-WebView concern PT-4031 anticipated; recommend a separate ticket. PT-4036/PT-4037 ticket text to be reconciled to the command-based design.

AI Involvement

AI-assisted — session. Brainstorming → spec → plan → subagent-driven implementation with per-task review, a whole-branch review, and human-gated design decisions (including re-hosting the command). All code was human-reviewed.

Risk level

Low — additive command + display-only conversion with pass-through fallbacks; degrades safely; the 11 extension consumers are unchanged; backward-compatible with old persisted scroll-group state.

🤖 Generated with Claude Code
https://claude.ai/code/session_01DyZeT3Mcy2WmGrkuCtnRaz

---

This change is 

---

Code Review Summary

Branch: pt-4036-versification-aware-sync

Base: origin/main

Date: 2026-07-01

Review model: Claude Opus 4.8

Files changed: 13

Overview

This branch makes scroll groups (synchronized scrolling) correct across projects that use
different versifications. Previously a scroll group stored a single Scripture reference and every
follower consumed it verbatim; a follower whose project used a different versification could land on
the wrong verse. The change tracks, per scroll group, the id of the project whose versification the
stored reference is expressed in ("source project"), persists it alongside the reference, and
converts the reference into each consumer's versification on read.

The conversion itself is a new project-agnostic PAPI command,
platformScripture.mapVerseRefBetweenProjects (verseRef, sourceProjectId?, targetProjectId),
implemented in C# core (VersificationConversionService) by grounding the reference in the source
project's ScrVers and delegating to libpalaso's ChangeVersification. It is best-effort: an
unknown source frame (null) or an unresolvable project returns the reference unchanged rather than
throwing. On the renderer side, scroll-group.service-host adds cached getScrRefForProject /
getScrRefForProjectSync (bounded session cache keyed on live versification identifiers, in-flight
coalescing, per-project versification subscription), and useScrollGroupScrRef takes an optional
consumer projectId, converting the followed reference for display via usePromise (synchronous
cached seed + async correction) with careful detach re-seeding. Consumers (toolbar, web view) pass
their project id.

API Changes

• @papi/core: ScrollGroupUpdateInfo — added optional sourceProjectId?: string field.
• scrollGroupService (IScrollGroupService, @papi/backend / @papi/frontend): setScrRef() —
  added optional trailing sourceProjectId?: string parameter.
• scrollGroupService: added method getScrRefForProject(scrollGroupId, projectId): Promise<SerializedVerseRef>.
• scrollGroupService: getScrRef() and onDidUpdateScrRef — documentation clarified only; no
  signature change (getScrRef still returns the raw stored reference).
• @papi/frontend/react: useScrollGroupScrRef() — added optional trailing projectId?: string
  parameter.
• platform-scripture.d.ts (papi-shared-types command): added
  platformScripture.mapVerseRefBetweenProjects(verseRef, sourceProjectId, targetProjectId): Promise<SerializedVerseRef>,
  marked @experimental.
• lib/platform-bible-react, lib/platform-bible-utils: no changes.

All public changes are backward-compatible: optional parameters are appended at the end of existing
signatures, the new ScrollGroupUpdateInfo field is optional, and getScrRefForProject is purely
additive. Existing two-arg setScrRef / no-arg getScrRef callers are unaffected.

Findings

Critical — Must address before merge

• CI-blocking react-hooks/exhaustive-deps error on convertScrRef in
  src/renderer/hooks/papi-hooks/use-scroll-group-scr-ref.hook.ts — the callback referenced the
  derived isConversionRequired const but did not list it, which the project config treats as an
  error (not a warning), so a clean CI checkout running eslint . would fail. This was
  pre-existing in the branch and not surfaced by the four analysis passes (they do not run lint); it
  was found during the post-change quality check. (fixed during review: inlined the gate condition
  if (!projectId || sourceProjectIdLocal === projectId) return scrRefLocal; so exhaustive-deps sees
  the real reactive inputs directly — no suppression, behavior identical, and the load-bearing
  sourceProjectIdLocal dependency is preserved. Adding isConversionRequired to the deps instead
  would make the rule then demand removing sourceProjectIdLocal, which drives the source-only
  re-conversion. Verified: typecheck + lint clean.)

[Author response: Author chose the inline-guard fix over a justified eslint-disable. Note for the
reviewer: this was pre-existing in the branch; the local worktree could not run eslint . natively
because it is nested under the parent repo (plugin-resolution ambiguity), so the error was only
visible via --resolve-plugins-relative-to .. Worth confirming CI is green after this fix.]

Important — Should address before merge

• Untested versification-subscription failure/retry path in
  src/renderer/services/scroll-group.service-host.ts (ensureVersificationSubscribed) — the
  catch block deletes the subscription entry so a project is not latched off for the session and a
  later call retries; the service-host tests always mocked pdpGet to succeed, so this deliberately
  resilient branch was never exercised. (fixed during review: added a test that rejects the first
  subscribe attempt per project then succeeds, using the same-versification fast path as observable
  proof that the failed entry was cleared for retry. Verified as a revert test — neutralizing the
  delete makes it fail.)

[Author response: Author asked for the test to be added during the interview.]

Minor — Consider

• isConversionRequired named like a boolean but not one in
  use-scroll-group-scr-ref.hook.ts — the is-prefix implies boolean, but projectId && ...
  evaluated to string | boolean | undefined. (fixed during review: changed to
  !!projectId && ..., a true boolean. Deliberately NOT Boolean(projectId) — verified with an
  isolated tsc check that Boolean(...) drops TypeScript's control-flow narrowing of projectId to
  string at the getScrRefForProject / getScrRefForProjectSync call sites, whereas !!projectId
  preserves it.)

• cacheConversion eviction untested in scroll-group.service-host.ts — the drop-oldest
  bound at CONVERSION_CACHE_MAX_SIZE = 1000 had no coverage. (fixed during review: added a test
  that fills to the cap + 1 and asserts the oldest entry is evicted while a newer one survives.
  Verified as a revert test. Side note surfaced while writing it: verse numbers > 999 saturate the
  BBBCCCVVV encoding used by compareScrRefs, so the test generates distinct refs by varying
  chapter+verse within valid field ranges — a test-data concern only, not a product issue.)

• C# InitializeAsync (command registration) untested in VersificationConversionService.cs
  — only MapVerseRefBetweenProjects was unit-tested, leaving the wire-name string and parameter
  docs unverified. (fixed during review: added a test asserting the command registers under its
  exact wire name command:platformScripture.mapVerseRefBetweenProjects, is marked experimental, and
  documents the three positional params verseRef, sourceProjectId, targetProjectId in order.)

• setScrRefSync inserts the new optional sourceProjectId? parameter before the existing
  optional shouldSetVerseRefSetting, shifting a positional slot (Author opted not to change;
  analyzers flagged it as low-risk — the export is renderer-internal, not a @papi/ surface, and all
  four in-repo callers were updated to the new arity. Appending would have been marginally less
  fragile for future edits.)

• useScrollGroupScrRef now carries three async-adjacent branches (sync seed + async display
  conversion + async detach re-seed) at the upper edge of single-hook readability; consider
  extracting the detach re-seed into a helper (Author opted not to change; analyzer noted this is a
  judgment call, not a defect. The logic is heavily commented and the subtleties are load-bearing.)

[Author response: Author selected findings 1, 4, and 5 to fix during review and left 2 and 3 as
acknowledged/low-risk.]

Template Propagation

Shared Regions Modified

None. No #region shared with <url> markers exist in any changed file.

Extension Config Changes

None. The only extensions/ file changed is
extensions/src/platform-scripture/src/types/platform-scripture.d.ts, a type-declaration file (not a
webpack.config.ts / tsconfig.json / package.json / .eslintrc*), so no template propagation is
needed.

Positive Observations

• No breaking @papi/ changes: every public modification is additive or an appended optional
  parameter; old getScrRef / two-arg setScrRef call paths retain identical behavior.
• New platformScripture.mapVerseRefBetweenProjects command is correctly declared as a type in
  platform-scripture.d.ts while registered by C# core — matching the established
  platformScripture.invalidateCheckResults precedent — and the TS declaration and C# implementation
  agree on parameter order and types. Marked @experimental on both sides.
• Strong, behavior-focused test coverage on both sides of the boundary: source-project tracking,
  persistence across reload, same-numbered-different-source, the platform.verseRef echo no-clobber,
  cache hits, concurrent-request coalescing, the share-versification fast path, and transient-failure
  fallback + retry; hook-side conversion, fallback, detach seeding (including mid-conversion re-seed),
  and re-conversion on a source-only change.
• VersificationConversionServiceTests.cs asserts against a direct libpalaso conversion (verifying
  wiring, not libpalaso's tables) and covers same-project, source-grounding, null-source, unresolvable
  source/target, HasMultiple bridge refs, and segment preservation — matching the best-effort
  contract.
• DRY across the sync/async boundary: planConversion + conversionCacheKey are shared by
  getScrRefForProject and getScrRefForProjectSync, with an explicit comment that the shared
  gate/key shape is what keeps the synchronous seed from missing the async-written cache.
• The C# VersificationConversionService follows the established command-handler-service pattern
  (internal class, primary-constructor PapiClient, InitializeAsync wired in Program.cs), and
  c-sharp/Projects/ is a defensible home for it.
• Display-only conversion with a synchronous seed and preserveValue: false avoids a visible flash of
  the wrong verse — good attention to perceived UI stability. The scroll-group interaction pattern is
  reinforced, not altered.
• Developer-facing logs (logger.warn, Console.WriteLine) are correctly not routed through
  localization; papi.d.ts appears regenerated rather than hand-edited.

Interview Notes

• Stated purpose: Make scroll groups compatible with different project versifications
  (PT-4036 / PT-4037).
• Working tree: Before the review the author committed the changes to be reviewed and reverted the
  rest, leaving a clean tree.
• Design walkthrough (detach async re-seed): Asked the author to explain why the detach path fires
  an async getScrRefForProject re-seed in addition to the synchronous displayScrRef seed. The
  author explained the mechanism well: use a synchronous cached value when available for an instant,
  correct display; on first visit the cache is empty so it falls back to the raw ref, and the async
  re-seed then corrects the now-independent ref if the resolved value differs. The author stated
  plainly: "Claude designed it, and I reviewed it." They can describe the general mechanism but did
  not independently surface the two finer points — the re-attach guard
  (scrollGroupIdLocalRef.current === undefined) that prevents a late async arrival from clobbering
  new state, and the "permanent freeze" rationale (once detached, the independent ref is treated as
  this project's own frame and never re-converted). These were supplied by the reviewer.
  • Author does not fully own: the detach re-seed logic in setScrollGroupId — AI-authored and
    author-reviewed. Recommend walking it end-to-end in the review meeting rather than treating it as
    settled.
• Source-project selection for a hand-set top-level reference (traced during review Q&A): When the
  toolbar's BCV control is set by hand, the sourceProjectId stamped on the scroll-group update — and
  therefore the frame every other follower converts from — is currentProject?.id. currentProject
  is resolved in use-project-picker-data.hook.ts as the first platformScriptureEditor.react web
  view (that has a projectId) encountered as rc-dock traverses the dock-layout tree
  (getAllOpenWebViewDefinitions → dockLayout.getAllWebViewDefinitions() →
  dockLayout.find(..., Filter.AnyTab), then .find(predicate) takes the first match). Key properties:
  • Only Scripture Editor windows qualify; resource viewers and other editor/webview types are skipped
    entirely — their versification never becomes the source.
  • The winner is dock-layout traversal order (structural tab position in the panel tree, docked and
    floating), NOT the focused/active tab, NOT most-recently-used, NOT most-recently-opened. It shifts
    if the user rearranges panels or closes that first editor. The only user-visible signal is the
    toolbar's currentProject.fullName (shortName) label.
  • If no Scripture Editor is open, currentProject is undefined, so sourceProjectId: undefined is
    sent and followers pass the reference through unconverted (unknown frame, deliberately not
    assumed English).
  • This currentProject selection pre-dates this branch (the picker already used it), but the branch
    gives it new weight: it now determines cross-project verse conversion, not just the picker label.
• Minor findings: Author chose to fix findings 1, 4, and 5 during the review and left 2 and 3 as
  acknowledged / low-risk (no code change).
• Wrap-up: Author had nothing further to flag.

In-Review Quality Check

Changes were made during the review (one production fix in the hook, three added tests). After
staging them, the following were run:

• npm run typecheck — passed (exit 0, no errors), including after the inlined-guard change that
  re-narrows projectId.
• ESLint (changed files) — clean (exit 0) after fixes. Two issues were fixed: the CI-blocking
  exhaustive-deps error (see Critical) and a paranext/require-disable-comment on a needed
  no-await-in-loop disable in the new eviction test (added a justification comment;
  no-await-in-loop genuinely fires because the sequential awaits are required for deterministic
  insertion-ordered eviction).
• Prettier (changed TS) and csharpier --check (changed C#) — both clean.
• Tests — use-scroll-group-scr-ref.hook.test.ts + scroll-group.service-host.test.ts: 24
  passed; VersificationConversionServiceTests: 8 passed. All three added tests were confirmed as
  genuine revert tests (they fail when the corresponding implementation line is neutralized).
• Tooling note (not a code issue): this worktree is nested under the parent paranext-core repo,
  so eslint . cannot run natively (it finds @typescript-eslint in both node_modules and aborts on
  ambiguity). Linting during this review used --resolve-plugins-relative-to . (the worktree's own
  plugins, same versions as the parent). A clean CI checkout is not nested and is unaffected — which
  is exactly why the pre-existing exhaustive-deps error would surface at CI.

Suggested Review Focus

• Detach re-seed in useScrollGroupScrRef.setScrollGroupId — AI-authored and author-reviewed.
  Walk through the first-visit cache-miss path, the async re-seed, the re-attach guard
  (scrollGroupIdLocalRef.current === undefined), and the "never re-converted after detach" freeze
  rationale to confirm the author can own it independently.
• Confirm CI is green after the exhaustive-deps fix — the error was only visible locally via
  a special eslint invocation due to worktree nesting; verify the clean-checkout CI lint passes.
• Source-of-truth for a hand-set toolbar reference's versification — with multiple Scripture
  Editors of different versifications open, the source frame stamped on the reference (and used to
  convert for every other follower) is whichever editor comes first in dock-layout traversal order,
  independent of which editor is focused; and it is undefined (unconverted passthrough) when no
  Scripture Editor is open. Now that this selection drives cross-project verse conversion — not just the
  picker label it originally fed — decide whether "first scripture editor in layout order" is the right
  source, or whether it should track the focused/active editor. (Full trace in Interview Notes.)
• setScrRefSync no-op guard semantics — the versification-blind compareScrRefs gate plus
  the sourceProjectId/shouldSetVerseRefSetting interactions (echo no-clobber, source-only change
  still emits but does not rewrite platform.verseRef). Confirm the intended matrix matches the
  implementation.
• Best-effort conversion contract — unknown source frame (null/undefined) is deliberately
  NOT assumed English and is passed through unconverted. Confirm this is the desired product behavior
  for restored/external platform.verseRef values whose frame is unknown.
• Mid-session versification change is out of scope — a versification change updates the cache
  key so the next conversion is fresh, but an already-displayed reference is not re-converted until the
  next navigation. Confirm this trade-off is acceptable.

[lyonsil]

Here is an example screenshot from P.B showing the versification in action. The Vulgate versification collapses Exodus 40:13-15 from other versifications into a single verse, Exodus 40:13. So when you click on 40:15 in one of the other translations using a different versification, it goes to 40:13 in Vulgate.

Note that 10 UI cannot match the 9 UI without a larger redesign, because the 9 UI includes a project indicator of which is the "active project" for versification purposes (and probably more things). In 10, there is no top-level project picker. Internally, the BCV control pays attention to which project set the verse reference. It someone just changes the BCV by hand in the top control, it keeps using the "active project" which is the one associated with the first open scripture editor (in layout order). If no editor has been opened yet, then it sends an empty project ID which should just use the raw (non-versified) verse reference in any consumers.

[rolfheij-sil]

Code review: versification-aware synchronized scrolling

Reviewed the new versification-aware sync in scroll-group.service-host.ts and use-scroll-group-scr-ref.hook.ts. The overall design holds together well — the shared planConversion gate keeping the sync and async paths from drifting, the versification-keyed conversion cache with in-flight de-duplication, and the seed-synchronously-then-correct-asynchronously pattern are all nicely thought through.

Seven things worth a look before merge (weight + type in the table, details inline). #2 and #3 aren't flat defects — they hinge on assumptions about how versification and project-less drivers behave in practice, so they're really questions about the intended contract.

#	Location	Weight	Type	Issue
1	scroll-group.service-host.ts:289	🟠 High	Bug	Same-versification fast path keys off the base ScrVersType only; projects sharing a base type but differing in custom.vrs skip conversion and mis-frame — a silent, persistent wrong verse in the mixed-versification case this feature targets.
2	scroll-group.service-host.ts:198	🟠 High*	Clarification / decision	A mid-session versification change refreshes the cache but never re-emits, so an on-screen reference stays mapped under the old versification indefinitely. Rests entirely on "versification never changes during a session."
3	scroll-group.service-host.ts:458	🟡 Medium*	Clarification / decision	A numbers-changed write from a project-less caller stamps sourceProjectId = undefined, wiping a known frame so other followers stop converting.
4	use-scroll-group-scr-ref.hook.ts:223	🟡 Medium	Bug	The post-detach async re-seed guards against the stale detach-time ref, so a navigation made during the conversion round-trip is silently reverted — a lost user action.
5	use-scroll-group-scr-ref.hook.ts:167	🟡 Medium	Transient UX	usePromise seed is the raw source-frame ref on a cache miss with preserveValue: false, so forward navigation flashes the unconverted verse (frequent — every uncached move).
6	use-scroll-group-scr-ref.hook.ts:160	🔵 Low	Transient UX	conversionSeed memo omits sourceProjectIdLocal, so a source-only authority flip briefly shows the reference converted from the previous source (rare).
7	scroll-group.service-host.ts:256	⚪ Minor	Cleanup	conversionCacheKey serializes the reference twice per navigation (the sync-seed and async paths each build the key).

* Conditional — see the inline note. #2 only bites if versification can actually change mid-session; #3 only if a project-less driver can share a scroll group with a differently-versified editor. Neither rises to Critical — all seven are recoverable display issues, no data loss or crash.

Details inline. Happy to discuss any of them.

[rolfheij-sil]

🟠 High · Bug

Same-versification fast path ignores custom.vrs

This disjunct skips conversion whenever sourceVersification === targetVersification. But those identifiers come from the platformScripture.versification setting, which is the base ScrVersType (a number, default 4) — not the project's actual custom.vrs content. Two projects can both report English (4) yet have custom versifications that merge or split verses differently.

When that happens, planConversion returns needsConversion: false on both the sync and async paths, so getScrRefForProject returns the raw source-frame reference unconverted and a follower lands on the wrong verse — a stable error until the next navigation, in exactly the mixed-versification case this feature exists to handle.

Is the versification identifier guaranteed to capture custom.vrs differences, or is it really just the base type? If it's the base type, this fast path isn't a safe conversion-skip — it would need the actual versification identity, or should just let the C# round-trip decide.

[lyonsil]

Fixed in 285f2c6 (with a stale-comment + papi.d.ts follow-up in 782548b). Confirmed the platformScripture.versification setting only carries the base ScrVersType (default 4), which a customized versification keeps (ScrVers.IsCustomized) — so two projects reporting the same identifier can still convert differently, and equal identifiers can't prove custom.vrs equality. Removed the disjunct from planConversion; the gate is now just unknown-source or source == target. The C# command decides for every cross-project pair (a genuinely-identical versification is a cached no-op), and the versification identifiers still key the conversion cache so a mid-session change re-keys.

[rolfheij-sil]

🟠 High (if the assumption is wrong) · Clarification / decision

A mid-session versification change updates the cache but never re-emits

This callback refreshes projectVersifications (keeping the fast path and the cache key fresh) but, per the comment just below, intentionally does not re-emit onDidUpdateScrRef. So a reference already on screen keeps displaying the verse mapped under the old versification until the user manually navigates — a stationary reader sees the wrong verse indefinitely.

The whole behavior rests on the stated premise that "a project's versification is chosen at creation and effectively never changes during a session." The platformScripture.versification setting is user-editable, though. If versification can in fact change mid-session (and other parts of the app react to it), this premise doesn't hold and the on-screen reference should re-convert rather than silently going stale.

Can you confirm whether that assumption is actually guaranteed? If it isn't, this needs to re-emit on change.

[lyonsil]

Confirmed the assumption isn't guaranteed — the platformScripture.versification setting is user-editable — so we handled it rather than relying on it (eab4257 host, e3438de hook). The host now emits an internal onDidChangeVersification event only on a genuine mid-session change (not the initial subscription load), and each consumer bumps a generation token that re-runs its conversion once. Blunt by design (every consumer re-converts on any versification change), which is fine because the change is rare and deliberate, and it avoids adding a per-consumer versification subscription. The event is host↔hook internal (not on the public NetworkEvents/service surface).

[rolfheij-sil]

🟡 Medium (if reachable) · Clarification / decision

A numbers-changed write from a project-less caller wipes a known source-project id (only bites if a project-less driver can share a group with a scripture editor)

On a numbers-changed write, scrRefSourceProjectIds[...] is set unconditionally to the caller's sourceProjectId. A driver with no project — e.g. a resource or dictionary panel whose saved web-view definition has no projectId — passes undefined, overwriting a previously-known source frame with undefined. After that, getScrRefForProject sees an unknown source and returns the raw reference, so a differently-versified follower stops converting and jumps to the raw verse, staying wrong until some project-bearing editor navigates the group again.

The no-op guard above deliberately lets a same-numbered / different-source write through (correct — the frame changed). But on a numbers-changed write whose incoming source is undefined, should the stored source id be cleared, or preserved? Preserving it when sourceProjectId === undefined would keep the known frame intact.

Flagging as conditional since it only matters if a project-less driver actually shares a scroll group with a differently-versified editor — let me know if that's reachable in practice.

[lyonsil]

This is by design. A write with no source project means the versification frame is genuinely unknown, so followers take the raw reference rather than mis-frame it under the previous source — the same deliberate passthrough as the platform.verseRef echo. It is reachable (a project-less web view sharing a group), but the pass-through-unconverted behavior is intended for an unknown frame. Made the contract explicit in-code in 66adcf2 (no behavior change).

[rolfheij-sil]

🟡 Medium · Bug

Post-detach re-seed can silently revert a user navigation

This guard checks only that the pane is still detached (scrollGroupIdLocalRef.current === undefined) and that the converted ref differs from the detach-time displayScrRef captured in the closure. It never checks whether the user has moved the pane since detaching.

Scenario: detach a pane at a verse whose conversion isn't cached (first visit). Line 206 seeds the raw source-frame ref and this IIFE fires the async conversion. Before it resolves, the user scrolls the now-independent pane to another verse. When the conversion lands, compareScrRefs(converted, displayScrRef) is still non-zero (converted ≠ the old detach-time verse), so setScrollGroupScrRefRef.current(converted) overwrites the user's new position and snaps the pane back to where it was detached — their navigation is lost.

The re-seed wants to fire only if the independent ref is still the one it seeded. Comparing the pane's current independent ref against the seeded displayScrRef (bail if the user has since changed it), rather than comparing converted against the stale displayScrRef, would close this.

[lyonsil]

The race is real in theory but needs two human actions — detach, then navigate the now-independent pane — both inside the millisecond conversion round-trip. A followed verse's conversion fires when the group ref changes (via usePromise), so by the time a user can operate the scroll-group selector it has already resolved and cached, and the async re-seed block isn't even entered in practice. Rather than harden an unreachable path, we deleted the async re-seed (7802b6b): detach now just keeps the currently-displayed (correct-frame) verse, and the residual is exactly the unreachable race, where the pane keeps what it was showing — recoverable, no persisted write.

[rolfheij-sil]

🟡 Medium · Transient UX

Forward navigation flashes the unconverted verse

On a cache miss, getScrRefForProjectSync (the conversionSeed on the line above) returns the raw source-frame reference, and with preserveValue: false the display resets to that seed while the async conversion is in flight. Forward navigation to a new reference is always uncached, so a differently-versified follower renders the source verse (e.g. Ps 147:1) for one round-trip before snapping to the correct verse (Ps 146:1) — a wrong-verse flash on essentially every move to a new reference.

There's a genuine tradeoff here: preserveValue: true would instead linger on the previous converted verse until the new one resolves (old position rather than wrong-numbered verse), or the mixed-versification case could withhold rendering until conversion completes. Each trades the flash for a different transient — worth deciding which is least jarring for readers.

[lyonsil]

Switched to preserveValue: true (a019bfb): the pane now lingers briefly on the previous converted verse while the next conversion is in flight, instead of flashing the raw source-frame verse. Judged the less jarring transient for readers, and it's first-visit-only (cached after). A true withhold-render would have meant adding an isConverting flag to the hook's return, updating every consumer, and giving the editors a loading affordance — not worth it for this transient.

[rolfheij-sil]

🔵 Low · Transient UX

conversionSeed memo omits sourceProjectIdLocal from its deps

The deps are [isConversionRequired, projectId, scrRefLocal] — sourceProjectIdLocal isn't listed. On a source-only change (same verse numbers set by a different-versification project), the memo isn't recomputed, so with preserveValue: false the display briefly resets to the seed computed from the previous source project's frame (a different verse) until convertScrRef re-runs and corrects it — a one-round-trip flash on authority flips.

convertScrRef already lists sourceProjectIdLocal as a dependency (line 152); adding it here would keep the seed consistent with the factory. The comment just above (lines 158–159) acknowledges the seed doesn't recompute on a source-only change — this is that gap made concrete.

[lyonsil]

Fixed in d6f3f86 — added sourceProjectIdLocal to the conversionSeed memo deps so the seed recomputes on a source-only change, consistent with convertScrRef.

[rolfheij-sil]

⚪ Minor · Cleanup

serialize(scrRef) runs twice per navigation

conversionCacheKey is built independently by both callers of planConversion — getScrRefForProjectSync (the sync seed) and getScrRefForProject (the async path) — so the same reference is serialized twice on every synchronized navigation, for every follower. Not a correctness issue, just repeated work on the sync-scroll hot path. If it's worth it, threading the plan (or its key) from the sync computation into the async call would avoid the second serialize.

[lyonsil]

Declining this one. Threading the key from the sync path into the async call would recouple the two paths that the shared planConversion deliberately keeps parallel (a divergent gate/key shape is exactly what would make the sync seed miss the async-written cache and reintroduce a flash). The cost is one small serialize of a verse-ref object per navigation — not worth the coupling.

[rolfheij-sil]

Re-review: round two

Thanks for the thorough turnaround on round one — dropping the fast path, the versification-change event, and the seed recompute all land well. I re-reviewed the full PR at the new head (96740ef). Two things block, plus one opportunistic fix and one doc line — my next pass will be scoped to the two blockers only, not another full sweep, so we can end the review loop here.

Blocking

1. c-sharp/Projects/VersificationConversionService.cs:69 — 🔴 Crash: every "return unchanged" fallback path NREs during response serialization.
A typical SerializedVerseRef has no versificationStr, so VerseRefConverter.Read builds a VerseRef with a null Versification. The fallback paths (null sourceProjectId, unresolvable project, conversion failure) return that ref, and VerseRefConverter.Write dereferences value.Versification.Type → NullReferenceException. So the command errors in exactly the cases the d.ts documents as "returned unchanged rather than throwing." The NUnit tests call the method directly and never serialize the result, which is why they pass.

Two ways out: (a) minimal — return the input SerializedVerseRef as-is on the fallback paths; (b) my preference — throw on failure instead of silently passing through. The TS caller's .catch already falls back to the raw ref and doesn't cache, so (b) also fixes a second latent issue for free: today's silent passthrough is indistinguishable from a genuine identity conversion, so getScrRefForProject caches a transient failure permanently (scroll-group.service-host.ts:437) — despite the adjacent .catch comment saying failures must not be latched. (b) needs the d.ts contract wording updated to match.

2. use-scroll-group-scr-ref.hook.ts:193 — 🟠 Regression: preserveValue: true displays stale verses, and the new conversionSeed recompute is display-inert.
usePromise reads defaultValue only in its mount initializer, and with preserveValue: true it never resets — so the recomputed seed added in d6f3f86 is never actually displayed after the first render. When isConversionRequired toggles false→true (source authority moves between differently-versified projects), the hook shows usePromise's last resolved conversion from an earlier phase. Repro: panes A/B share group 0 → navigate A to R1 (B shows conv(R1)) → navigate in B to R2 → navigate A to R3 → B first visibly jumps back to conv(R1) before landing on conv(R3); a detach during that window persists the stale ref into the web view definition. Fix direction: make the display fall back to the recomputed conversionSeed instead of usePromise's frozen internal value — which also makes the seed machinery you built actually execute.

While you're in there (non-blocking)

3. use-scroll-group-scr-ref.hook.ts:235 — the "unreachable" justification for removing the detach re-seed doesn't hold on the conversion-failure path: convertScrRef catches, falls back to the raw source-frame ref, caches nothing, and nothing retries until the user navigates. A detach in that state persists an unconverted ref into the web view definition — permanently, across restarts. A guarded re-seed (only applied if the user hasn't navigated since detach) would fix this and round one's lost-navigation defect at the same time. Fixing #2 shrinks this window a lot, so treat it as opportunistic.

4. Doc line — the narrowed no-op guard (scroll-group.service-host.ts:493) means onDidUpdateScrRef now fires on same-numbers, source-only writes. Worth one sentence in the event's jsdoc so external consumers don't keep assuming "fires only when the verse actually changed."

Details available on any of these. #1 and #2 are the only things between this and approval from my side.

[lyonsil]

Thanks for the thorough round two. Both blockers are fixed, along with the two non-blocking items and one extra issue a self-review turned up. Pushed to 117404c (main change in 5b9b831).

#1 — C# NRE + cache-latch — fixed. Went with your option (b). The fallback paths now throw VersificationConversionException on genuine failures (unresolvable named source/target, mapping failure); the null-source passthrough stays. Also hardened VerseRefConverter.Write to null-guard Versification — defense-in-depth, and it's what keeps the null-source passthrough serializable. Throwing routes the caller's .catch to the raw-ref fallback, which does not cache, so the transient-failure latch is gone too. Updated the d.ts contract + C# docs to say it throws, and added a serialization-path test (the NUnit tests passed before only because they never serialized the result).

#2 — stale display / inert seed — fixed. The display now lingers on the last-displayed ref: each async conversion is tagged with the inputs it was computed for (verse, source, target, versification generation) and shown only while current; otherwise it lingers on the verse that was last on screen, falling back to the sync seed only at cold start. That removes the visible jump back to conv(R1) and makes the seed machinery actually drive the display. Your repro is now a regression test. (Trade-off note: this deliberately revises the old "flash vs linger" call toward "never show a stale unrelated verse" — a brief raw-frame lag on an uncached forward move is the accepted cost, and it's invisible except on verses where the versifications actually diverge.)

#3 — detach re-seed — documented, not re-added. Per your "opportunistic / #2 shrinks the window" note, I did not re-introduce the post-detach async re-seed. #2 makes the detach seed the last-displayed (correctly-framed) value; the only residual is a detach during a genuine conversion failure, where the raw ref is the honest best-effort (a re-seed would fail identically). Documented that at the detach site.

#4 — doc line — done. Added a sentence to onDidUpdateScrRef (and its emitter summary) noting it also fires on same-numbers, source-only writes.

Bonus (self-review): the conversion tag built for #2 initially omitted the target projectId, so a target-project change (projectId is mutable on a live web view) could briefly show the conversion into the previous target's versification. Fixed — the tag now mirrors the full input set — with a regression test, plus tests for detach-mid-linger and failure-mid-linger.

Two things I considered and left as-is, happy to revisit either:

• Throw vs. typed result: kept your (b). The throw fixes the cache-latch and the sole caller catches-and-ignores (raw fallback), so a typed {converted} | {notConverted} return would add API surface no consumer reads.
• conversionSeed is now cold-start-only, but I kept it for the no-flash-on-revisit behavior; corrected its (now-moot) memoization comment.

AI-assisted — session

[rolfheij-sil]

🤖 Posted by Claude Code on Rolf's behalf — this is an automated max-effort re-review, not Rolf's hand-review. Nothing below is blocking. It's a second opinion for the record before approval.

Automated re-review at 117404c

Both round-two blockers verify as genuinely fixed. The C# NRE fix (VerseRefConverter.Write null-guard) is a pure defensive addition — the non-null path is byte-identical, so there's no blast radius on the shared serializer — and the throw-on-failure routes the caller's non-caching fallback correctly. The stale-display fix's input-tag currency check (verse + source + target + generation) captures everything that determines the conversion output. No new crash, no data loss.

Four residuals surfaced, all recoverable and non-blocking. Three are the cost side of tradeoffs already chosen in the earlier rounds — flagging them mainly because in two cases the written justification claims a cleanness the code doesn't quite have.

1. 🟠 Same-versification followers lag one round-trip per navigation — use-scroll-group-scr-ref.hook.ts:246 (verified)

convertedDisplay = currentConversionRef ?? lastDisplayedRef.current ?? conversionSeed. Since lastDisplayedRef precedes conversionSeed, a follower on an uncached verse shows the previous verse for a full C# round-trip before jumping to the new one — even when the two projects share a versification and the seed already holds the correct new verse. This hits the primary sync-scroll case (two English-versification editors, one scroll group): every uncached navigation trails the driver by one round-trip.

This is the flip side of the round-one #5 flash-vs-linger decision, and you genuinely can't prefer the seed for the same-versification case without reintroducing the flash for divergent versifications (no synchronous equality signal — that's the removed fast path). So not a defect to "fix" — but the round-two note "invisible except where versifications diverge" doesn't hold: for same-base-versification, different-project followers it's visible on every uncached move. Worth an explicit call on whether that per-navigation lag is acceptable for the common case.

2. 🟡 A failed conversion strands an attached follower on the raw frame until next navigation — use-scroll-group-scr-ref.hook.ts:192 (plausible)

If mapVerseRefBetweenProjects isn't registered yet (startup) or fails transiently, the host resolves to the raw source-frame ref, convertScrRef tags it current, and the currency check displays it. Nothing re-fires convertScrRef when the command later comes online, so a divergent-versification follower shows a mis-framed verse until the user navigates. The line-193 comment already accepts "retried on the next navigation" — so this is the documented limitation extended from the detach case (round-two #3) to the attached case. Sharpest trigger is the startup window before the extension registers the command. Question: is a mis-framed first-displayed verse (until first navigation) acceptable there?

3. 🟡 Conversion cache keyed only on base ScrVersType; a custom.vrs change serves a stale mapping — scroll-group.service-host.ts:254/307 (plausible)

Round-one #1 one layer down: the conversion now uses the real ScrVers, but the cache key and its onDidChangeVersification invalidation both derive from platformScripture.versification = the base type only. If a project's custom.vrs changes mid-session while the base type stays (e.g. 4), the setting value doesn't change → changed stays false → no invalidation → a revisit to the same ref serves the mapping computed under the old custom.vrs. The round-one reply "the versification identifiers still key the conversion cache so a mid-session change re-keys" holds only for base-type changes, not custom.vrs. Reachability is narrow (needs a runtime custom.vrs edit with an unchanged base type — not a normal flow), so low priority — but it's a real hole in the invalidation invariant. Confirm unreachable, or document it.

4. 🔵 Two hook tests exercise a path production can't reach — use-scroll-group-scr-ref.hook.test.ts:108/389 (verified, cleanup)

Both mockRejectedValue on getScrRefForProject, but the real host getScrRefForProject catches internally and always resolves to the raw ref (never rejects). So they validate the hook's defensive catch (line 192) — effectively dead under the current contract — rather than the real failure flow, which is already covered in scroll-group.service-host.test.ts. Deletable with no production behavior change.

Also considered and dismissed

TryGetVersification swallowing registration errors for resource panes (degrades to the intended raw-ref fallback, not a defect); onDidUpdateScrRef firing on source-only writes (jsdoc added in round-two #4); the isConversionRequired/convertScrRef gate duplication (intentional, documented for exhaustive-deps narrowing).

---

None of the above blocks. #1 and #3 are the two worth a sentence back — not as bugs, but to reconcile the wording with the behavior before this merges.

AI-assisted re-review — session

[rolfheij-sil]

Found some none blocking things.
So I'm approving the PR

[rolfheij-sil]

@rolfheij-sil reviewed 16 files and all commit messages, and resolved 7 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved.

[lyonsil]

From Claude for 1 - same-versification round-trip lag - you're right, accepting it; wording corrected.

Agreed, and the round-two note overreached. "Invisible except where versifications diverge" is wrong: because isConversionRequired gates on project identity, not versification identity, two different-project followers always take the async path even when their versifications are identical. So for the primary case - two same-base-versification editors in one scroll group - every navigation to an uncached verse shows the prior verse for one mapVerseRefBetweenProjects round-trip before catching up. That's the common case, not an edge.

It stays as-is by design. The display order (currentConversionRef ?? lastDisplayedRef.current ?? conversionSeed) deliberately prefers lingering on the last-displayed verse over the synchronous seed, because for divergent versifications that seed is the raw source-frame ref - the wrong verse - and preferring it is exactly the flash the round-two stale-display fix removed. There's no synchronous versification-equality signal to tell the two cases apart (that was the fast path removed in round two), so both pay the same one-round-trip linger.

I'm accepting that cost: the lag is bounded to a single round-trip, self-corrects on arrival, never displays a wrong verse, and the cache warms so revisits are instant. Recovering the same-versification case would mean reintroducing a synchronous same-versification check, which reopens the divergent-versification flash. The code comment at the display line already frames this honestly ("sluggish-but-stable beats flashing a wrong-versification verse"); only the PR note claimed invisibility. No code change - flagging the corrected wording for the record.

For 2 - I'm not worried about this particular window. If the user is navigating and that method hasn't registered yet at startup, there are much larger problems happening in the application.

For 3 - This is a very rare case (modifying a custom versification mid-session). Not worth digging into right now.

For 4 - This is minor and not worth extending the PR.