[P3] markers-checklist: follow-up T1-stage-2 - Versification NetworkObject → projectInterface on ParatextProjectDataProvider

[rolfheij-sil]

Code Review Summary

Branch: ai/feature/markers-checklist-versification-pdp-rolf-05-11-2026

Base: origin/ai/main

Date: 2026-05-12

Review model: Claude Opus 4.7

Files changed: 9

Overview

This is the T1 stage 2 follow-up to TJ Couch's review of merged PR #2219 (markers-checklist). It removes the IVersificationService NetworkObject and re-exposes its three lookup methods (lookupFinalVerseNumber, lookupFinalChapter, lookupFinalVerseNumbersInBook) directly on ParatextProjectDataProvider under a new 'platformScripture.Versification' projectInterface — Option B from the design study, mirroring the existing GetMarkerNames pattern. The redundant projectId argument is dropped from each method.

In-tree consumers (checklist.web-view.tsx, platform-scripture-editor.web-view.tsx) are migrated in the same commit. The standalone VersificationService.cs is deleted along with its Program.cs instantiation. Eight existing tests are ported from VersificationServiceTests.cs to a renamed ParatextProjectDataProviderVersificationTests.cs that mirrors the sibling ParatextProjectDataProviderCommentTests pattern. No user-visible UI changes.

API Changes

• extensions/src/platform-scripture/src/types/platform-scripture.d.ts:
  • Removed export type IVersificationService (and the implicit 'platformScripture.versificationService' network-object wire surface).
  • Added export type VersificationProjectInterfaceDataTypes = Record<string, never> (auxiliary-only projectInterface — first one in this module to use the empty-data-types pattern; rationale documented inline).
  • Added export type IVersificationProjectDataProvider — extends IProjectDataProvider<VersificationProjectInterfaceDataTypes> with lookupFinalVerseNumber(bookNum, chapterNum), lookupFinalChapter(bookNum), lookupFinalVerseNumbersInBook(bookNum). All three drop the previous projectId first-arg.
  • Added 'platformScripture.Versification': IVersificationProjectDataProvider to the papi-shared-types ProjectDataProviderInterfaces map.
• c-sharp/Projects/ProjectInterfaces.cs: added VERSIFICATION = "platformScripture.Versification".
• c-sharp/Projects/LocalParatextProjects.cs: added ProjectInterfaces.VERSIFICATION to s_paratextProjectInterfaces.
• c-sharp/Projects/ParatextProjectDataProvider.cs: registered three new RPC functions (lookupFinalVerseNumber, lookupFinalChapter, lookupFinalVerseNumbersInBook) in GetFunctions().
• c-sharp/Projects/VersificationService.cs: deleted. The object:platformScripture.versificationService network object is removed from the wire surface.
• c-sharp/Program.cs: dropped the VersificationService instantiation and InitializeAsync call.
• papi.d.ts, lib/platform-bible-react/, lib/platform-bible-utils/: unchanged.

Findings

Critical — Must address before merge

• Removal of IVersificationService / platformScripture.versificationService is a breaking change to a public extension API surface (shipped in merged PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219). External extensions consuming it via papi.networkObjects.get<IVersificationService>(...) would error on both runtime and compile. (Author confirmed: this surface never escaped paranext-core — the markers-checklist work is Paratext-internal and the versification service was newly minted in [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 with no third-party adopters. Safe to remove in one PR.)

Important — Should address before merge

• Malformed @example JSDoc on lookupFinalVerseNumbersInBook (extensions/src/platform-scripture/src/types/platform-scripture.d.ts:843-845) — prettier-plugin-jsdoc reflowed the example as prose: const was capitalized to Const (invalid TypeScript), and three example statements were collapsed onto shared lines. (Fixed during review: replaced with a fenced ```typescript code block matching the established pattern used elsewhere in this file, e.g. engine.beginFindJob. Prettier verified to not re-flow the new form.)
• Versification tests skip RegisterDataProviderAsync() in setup, unlike the sibling ParatextProjectDataProviderCommentTests.cs which calls it at line 57. Compliance analyzer worried this would let a future drop of a retVal.Add(...) line in GetFunctions() silently break the wire surface while tests still pass. (Author opted for status quo after deeper investigation: adding RegisterDataProviderAsync() symmetrically wouldn't actually catch the analyzer's regression — registration would still succeed if one tuple were removed; it just wouldn't register that function. The Comment tests have the same blind spot. The pattern that would catch the regression is the CAP-012 ManageBooksServiceRegistrationTests style — call RegisterDataProviderAsync(), then assert Client.RegisteredRequestTypes contains each expected object:{name}-data.{methodName}. For 3 dead-simple passthrough methods with end-to-end live-app smoke-test coverage in the PR description and 8 existing off-by-one tests, the defensive value didn't justify the new test fixture.)

Minor — Consider

• VersificationProjectInterfaceDataTypes = Record<string, never> is novel in this module — no other *ProjectInterfaceDataTypes declaration uses an empty record. JSDoc above explains the rationale. (Author decision: acceptable as-is; rationale is documented inline.)
• Stale cross-file line-number references in checklist.web-view.tsx — the block-header comment at line 434 said "Mirrors platform-scripture-editor.web-view.tsx:351-377" but the editor block now spans ~351-373. A second stale reference at line 462 pointed to "scripture-editor.web-view.tsx:374's [chapterNum] access pattern" but line 374 is now the deps array, not the [chapterNum] access (which is at line 370). (Fixed during review: rewrote both to refer to the block/expression by name rather than line number, so they don't drift again.)
• IVersificationProjectDataProvider's JSDoc directs callers that cache results to subscribe to the 'platformScripture.versification' project setting. Neither in-tree consumer (checklist.web-view.tsx or platform-scripture-editor.web-view.tsx) does — both use usePromise(fetchLastVersesInCurrentBook, [versificationPdp, currentBookNum]), so the cached result only invalidates on book-change or PDP re-acquisition, not on a runtime versification-setting change. (Author decision: leave as-is. The gap is pre-existing — the previous IVersificationService had no subscription mechanism at all, so this PR actually improves the situation by giving consumers a documented way to subscribe via the PDP. Real-world impact is low: both UIs only care about the current book's verse counts, so a book switch refreshes anyway. Subscribe wiring can be added if/when the niche "user changes versification mid-session without switching books" scenario surfaces.)

Three additional stylistic observations were flagged by analyzers but did not require action and were not raised in the interview:

• Repeated var scrText = LocalParatextProjects.GetParatextProject(ProjectDetails.Metadata.Id); boilerplate in the new C# methods — consistent with the established convention in ParatextProjectDataProvider.cs (~21 occurrences across the class).
• One-line for loop body without braces in LookupFinalVerseNumbersInBook — consistent with surrounding C# style.
• Mixed-casing between projectInterface name ('platformScripture.Versification') and setting key ('platformScripture.versification') is a copy-paste footgun, but it follows the existing PascalCase-vs-camelCase convention.

Template Propagation

Shared Regions Modified

None. (None of the changed files contain #region shared with markers.)

Extension Config Changes

None. (git diff --name-only shows no changes to any package.json, tsconfig*.json, webpack.config.*, .eslintrc*, .erb/configs/, or .github/workflows/ files.)

Positive Observations

• The refactor faithfully mirrors the existing MarkerNames PDP pattern referenced in the PURPOSE — same IProjectDataProvider<…DataTypes> & { … } shape, same GetFunctions() registration, same ProjectInterfaces.VERSIFICATION constant and LocalParatextProjects._interfaces advertisement.
• Both in-tree consumers migrated in the same commit; zero residual references to IVersificationService, versificationService, or platformScripture.versificationService anywhere in the workspace.
• The deletion of VersificationService.cs is matched cleanly in Program.cs (both the construction and InitializeAsync() call are removed).
• New #region Versification (platformScripture.Versification) block in ParatextProjectDataProvider.cs names the projectInterface explicitly — making code-to-projectInterface mapping easy to find, and an improvement over the existing #region USFM / #region Comments style.
• The renamed test file ParatextProjectDataProviderVersificationTests.cs follows the naming convention of the sibling ParatextProjectDataProviderCommentTests.cs, uses [ExcludeFromCodeCoverage] correctly, and the class-level summary documents what is and isn't worth testing. Good coverage of the off-by-one boundary in LookupFinalVerseNumbersInBook (length, index 0, index 1, last index, exhaustive parallel comparison) plus a single-chapter Philemon edge case.
• The new JSDoc on lookupFinalVerseNumbersInBook documents the 1-based indexing contract that the old version under-documented ("Index 0 is a filler 0; the returned array has length lastChapter + 1"), reducing off-by-one risk for new callers.
• JSDoc clearly documents in-session freshness semantics and recommends subscribing to the 'platformScripture.versification' setting for change notifications — preempting an obvious follow-up question. The setting key is verified to exist (c-sharp/Services/ProjectSettingsNames.cs, projectSettings.json).
• Casing discipline is correct and intentional: 'platformScripture.Versification' (uppercase V) is the projectInterface name; 'platformScripture.versification' (lowercase v) is the project setting — both kept distinct and used consistently.
• The TS callsites are correctly migrated: useProjectDataProvider('platformScripture.Versification', projectId) replaces papi.networkObjects.get<IVersificationService>(...), the versificationPdp guard replaces the projectId guard, and useCallback deps now correctly track versificationPdp instead of projectId.
• No edits to papi.d.ts; the auto-generated PAPI surface is correctly left alone.
• No user-visible strings introduced (only logger.debug diagnostics, correctly excluded from localization).
• No new dependencies, no build-config changes, no CI/workflow changes.

Interview Notes

• Stated purpose (from PR body): T1 stage 2 architectural refactor addressing TJ Couch's deeper feedback on merged PR [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 — convert the IVersificationService NetworkObject to a per-project-PDP 'platformScripture.Versification' projectInterface (Option B from the design study), mirroring the GetMarkerNames pattern. Companion to [P3] markers-checklist: follow-up - TJ review #2254 (small follow-up) and [P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages) #2258 (Sebastian's UX-2 review, stacked on [P3] markers-checklist: follow-up - TJ review #2254).
• Why Option B over a TS-layering PDPE wrapper: PR description lists three reasons — no existing precedent for "TS PDPE wraps a NetworkObject" (every layering PDPE in the codebase does pure PDP composition); ParatextProjectDataProvider already mixes auxiliary methods like createComment, canUserAssignThread, getMarkerNames so versification fits naturally; the PDP is per-project so the projectId arg becomes redundant.
• Critical "breaking API" finding dismissed: author confirmed the IVersificationService surface never escaped paranext-core. The markers-checklist work is in-development and the versification service was newly added in [P3][backend+ui] markers-checklist: Backend + UI implementation (C# data provider, React web view, E2E tests) #2219 with no third-party adopters. Removal in a single PR is safe.
• Important "test registration plumbing" finding dismissed after deeper investigation: the analyzer's proposed fix (add RegisterDataProviderAsync() symmetrically with Comment tests) wouldn't actually catch the regression it was worried about — registration would still succeed if one tuple were dropped from GetFunctions(). The pattern that would catch it (CAP-012 ManageBooksServiceRegistrationTests — assert Client.RegisteredRequestTypes contains each expected wire name) wasn't worth the new fixture for 3 dead-simple passthrough methods already covered by 8 off-by-one tests and the live-app smoke test documented in the PR description.
• Minor "subscribe contract un-honored" finding dismissed: author noted the gap is pre-existing (the previous IVersificationService had no subscription mechanism either, so this PR strictly improves the situation by enabling subscription); real-world impact requires the niche case of "user changes versification mid-session without switching books", which neither UI currently encounters in practice.
• The author demonstrated clear design understanding throughout the interview — crisp, decisive responses; no deferrals to AI; no "I'm not sure"s.

In-Review Quality Check

Two in-review edits, both pure JSDoc/comment changes with zero code-path impact:

1. extensions/src/platform-scripture/src/types/platform-scripture.d.ts:843-852 — replaced malformed @example block with a fenced ```typescript code block.
2. extensions/src/platform-scripture/src/checklist.web-view.tsx:432-436 and 462-465 — rewrote two stale cross-file line-number references to refer to the block/expression by name.

Verification performed:

• Prettier: npx prettier --check on both files — clean (confirms prettier-plugin-jsdoc does not re-flow the new fenced code block, which was the original failure mode of the malformed @example).
• TypeScript typecheck / ESLint / test suite: not run. The worktree was created without npm install (a full install for a comment-only review would be disproportionate). Both edits are JSDoc/comment-only and cannot affect type checking, lint output beyond formatting, or test results. The risk is effectively zero, but flagging the gap for transparency — the author should run npm run typecheck && npm run lint && npm test once before pushing if they want absolute confirmation.

Suggested Review Focus

Prioritized areas for the author-reviewer meeting:

• Empty-data-types pattern: VersificationProjectInterfaceDataTypes = Record<string, never> is the first auxiliary-only projectInterface in platform-scripture.d.ts. Worth flagging to the reviewer in case the team wants to standardize on a different convention for future auxiliary-only PDPs (e.g. inline IProjectDataProvider<Record<string, never>> vs. a named alias).
• Subscribe contract gap: the d.ts JSDoc says callers caching results should subscribe to 'platformScripture.versification'. Neither in-tree consumer does. Author judged the gap is pre-existing and low-impact, but the reviewer may want to confirm whether to add the subscribe wiring now or accept the documented-but-deferred contract.
• Wire-registration test coverage: the PR has no regression test that would catch a dropped retVal.Add(...) line in GetFunctions(). The author deferred this defensive coverage as not worth the fixture given the methods' simplicity. The reviewer may want to confirm or push back, and if push back, the CAP-012 ManageBooksServiceRegistrationTests pattern is the recommended template.
• External-consumer confirmation: the analyzer flagged removal of IVersificationService as a breaking public-API change. Author confirmed it never shipped externally — worth a quick second confirmation from the reviewer (or whoever has visibility into downstream extensions / paranext-extension-template) before merge.

---

Merge sequencing: This PR is part of a 3-PR follow-up to PR #2219:

• [P3] markers-checklist: follow-up T1-stage-2 - Versification NetworkObject → projectInterface on ParatextProjectDataProvider #2256 (this PR) — T1 stage 2 architectural refactor (Versification NetworkObject → projectInterface).
• [P3] markers-checklist: follow-up - TJ review #2254 — TJ review style/docs fixes, sibling, independent.
• [P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages) #2258 — Sebastian UX-2 review (stacked on [P3] markers-checklist: follow-up - TJ review #2254's branch).

Either #2254 or #2256 can merge first; the other will need a rebase since they touch overlapping files (platform-scripture.d.ts, checklist.web-view.tsx). After both merge, #2258 may also need a rebase before merging.

Summary

Second follow-up to TJ Couch's review on merged PR #2219 (markers-checklist). Companion to #2254 (small follow-up).

T1 stage 2 — converts the IVersificationService NetworkObject to a per-project-PDP 'platformScripture.Versification' projectInterface, directly addressing TJ's deeper architectural feedback:

these methods should actually go in a projectInterface and go in a PDP instead of just being their own thing. Each one starts with projectId; that's a pretty dead giveaway that the whole service should just be a projectInterface

Approach

Implemented as Option B from the design study — versification methods live directly on ParatextProjectDataProvider (mirrors the GetMarkerNames pattern), exposed as a new projectInterface. The standalone VersificationService NetworkObject is removed entirely.

Why Option B rather than a TS layering PDPE around the existing service:

1. No precedent for "TS PDPE wraps a NetworkObject" — every layering PDPE in the codebase (ScriptureFinder, ScriptureExtender, lexical reference) does pure PDP composition. A TS wrapper would be a novel pattern.
2. ParatextProjectDataProvider already mixes get/set/subscribe with auxiliary methods (createComment, canUserAssignThread, getMarkerNames) — versification lookups fit the same shape naturally.
3. ParatextProjectDataProvider is per-project; the projectId argument becomes redundant. The consumer API gets cleaner.

Changes

C# (backend)

• ProjectInterfaces.cs — add VERSIFICATION = "platformScripture.Versification"
• LocalParatextProjects.cs — register VERSIFICATION in s_paratextProjectInterfaces
• ParatextProjectDataProvider.cs — add LookupFinalVerseNumber, LookupFinalChapter, LookupFinalVerseNumbersInBook methods (sourced from the deleted service); register in GetFunctions(). Methods read scrText.Settings.Versification fresh on each call so the projectInterface stays correct under runtime versification changes.
• Program.cs — drop VersificationService instantiation + InitializeAsync call.
• VersificationService.cs — deleted.

TypeScript (frontend / contracts)

• platform-scripture.d.ts — define IVersificationProjectDataProvider extending IProjectDataProvider; register under ProjectDataProviderInterfaces. Remove IVersificationService.
• checklist.web-view.tsx + platform-scripture-editor.web-view.tsx — swap papi.networkObjects.get<IVersificationService>(…) for useProjectDataProvider('platformScripture.Versification', projectId). Drop the redundant projectId argument from each method call.

C# tests

• Rename VersificationServiceTests → ParatextProjectDataProviderVersificationTests. Construct via DummyParatextProjectDataProvider (mirrors ParatextProjectDataProviderCommentTests). Drop the projectId arg from each method call. Drop the "unknown projectId" test — that's now a PDP-resolution failure earlier in the pipeline, not the responsibility of these methods.

Verification

• C# tests — dotnet test: 704 passing, 6 skipped. 8 versification tests pass after the refactor; 0 regressions in the broader suite.
• TypeScript typecheck — npm run typecheck: clean across all workspaces.
• TypeScript lint — npm run lint: 0 errors (pre-existing warnings unchanged).
• TypeScript tests — npm test: 1612 passing; 5 pre-existing storybook FAIL files are unrelated (storybook-cache export issue, same on ai/main).
• Runtime smoke test via PAPI on the live app:
  • Old object:platformScripture.versificationService.* methods no longer appear in rpc.discover (NetworkObject correctly de-registered).
  • Each Paratext project PDP now exposes lookupFinalChapter, lookupFinalVerseNumber, lookupFinalVerseNumbersInBook on object:<projectId>-pdp-data.*.
  • Live calls return correct values: lookupFinalChapter(GEN) → 50, lookupFinalVerseNumber(GEN, 1) → 31, lookupFinalVerseNumbersInBook(PHM) → [0, 25].

Relationship to #2254

#2254 (small follow-up — Group A) ships docs/type fixes for the remaining TJ-feedback items. These two PRs are independent and can merge in either order — they touch disjoint code paths, so no rebase conflict either way.

Test plan

• C# tests pass
• TS tests pass
• Runtime PAPI smoke test confirms registration and behavior
• CI green
• Manual: open Markers Checklist or Scripture Editor; confirm the verse-grid pickers still work (they pull from the new projectInterface under the hood)

🤖 Generated with Claude Code

---

This change is 

[rolfheij-sil]

Closed inadvertently when the head branch was renamed to the canonical ai/feature/markers-checklist-rolf-05-11-2026-versification-pdp via the GitHub branch-rename API. No reviews or comments had been posted on this PR.

Replaced by #2263 — same commits, same diff, canonical branch name so automated status reports pick it up.