Split published Paratext projects into a dedicated PDP factory

[tjcouch-sil]

Code Review Summary

Branch: ai/feature/paratext-published-pdpf-split-tj-05-27-2026

Base: origin/ai/main

Date: 2026-05-28

Review model: Claude Opus 4.7

Files changed: 11

Overview

This PR splits published Paratext projects (ScrText.IsResourceProject == true — ResourceScrText and JoinedScrText) into a dedicated PDP factory ParatextPublishedProjectDataProviderFactory so the wire surface honestly reflects what those projects can do: published projects are read-only at the storage layer and cannot accept comment writes, so the new factory advertises every projectInterface the regular factory does EXCEPT legacyCommentManager.comments. The 12 comment wire methods are also no longer registered on the PDP itself when its advertised interfaces do not include LEGACY_COMMENT, so the wire-level contract and the PDP method surface stay in lockstep. Both factories cross-reject projects belonging to the sibling factory, and the wire-surface change is verified by new integration tests.

During the review, the two factories were refactored to share an abstract base (ParatextProjectDataProviderFactoryBase) so PDP-cache management, double-checked-locking creation, and the existing-PDP lookup live in one place instead of being duplicated. The cross-factory rejection discriminator was also moved from the derived LEGACY_COMMENT membership check to ScrText.IsResourceProject directly, which protects the published factory's documented contract from future drift (e.g. a non-resource project type that later drops LEGACY_COMMENT won't slip in). Several smaller cleanups (dead-code removal, doc-comment improvements, lazy initialization of CommentManager, test consolidation) were folded in.

API Changes

• No changes to TypeScript public API surfaces (lib/platform-bible-react/, lib/platform-bible-utils/, papi.d.ts, extension *.d.ts files).
• PAPI wire surface (C# → PAPI, visible to extensions):
  • Published Paratext projects (ScrText.IsResourceProject == true) no longer advertise the legacyCommentManager.comments projectInterface.
  • Published Paratext PDPs no longer register the 12 comment wire methods: getCommentThreads, createComment, addCommentToThread, deleteComment, updateComment, setIsCommentThreadRead, findAssignableUsers, canUserCreateComments, canUserAddCommentToThread, canUserAssignThread, canUserResolveThread, canUserEditOrDeleteComment.
  • A new PDP factory platform.ParatextPublished-pdpf is registered alongside the existing platform.Paratext-pdpf.
• Internal C# API:
  • New internal abstract class ParatextProjectDataProviderFactoryBase — shared base for both Paratext PDP factories.
  • ParatextProjectDataProviderFactory and ParatextPublishedProjectDataProviderFactory both inherit from the new base; concrete classes are now ~30 lines each.
  • LocalParatextProjects.GetAvailableUnpublishedProjectDetails() and GetAvailablePublishedProjectDetails() — new partition helpers (use scrText.IsResourceProject as the discriminator).
  • LocalParatextProjects.GetAllProjectDetails() — refactored to GetAvailableUnpublishedProjectDetails().Concat(GetAvailablePublishedProjectDetails()) so the partition helpers are the single source of truth.
  • LocalParatextProjects.GetParatextProjectInterfaces(bool isPublished) — new signature. The previous no-arg overload was removed (it had no production callers after the partition).
  • LocalParatextProjects.LoadProjectDetails — removed (private, unused dead code).
  • ParatextProjectDataProvider._commentManager — now Lazy<CommentManager> so published PDPs avoid the cost of CommentManager.Get (whose value they never read).

Findings

Critical — Must address before merge

None.

Important — Should address before merge

• Wire-surface change is observable to existing extension call sites of useProjectDataProvider('legacyCommentManager.comments', projectId) (verified during review: all callers handle the absent PDP gracefully — comment-list.web-view.tsx uses a withPdp helper that returns safe defaults; platform-scripture-editor.web-view.tsx null-checks commentsPdp in every consumer and the context-menu "Insert comment" item is isDisabled: !canUserCreateComments; the USJ overlay PDP factory's projectInterfacesToLayerOver requires legacyCommentManager.comments so the cascade for published projects is automatic; the insertCommentAtSelection controller method's existing try/catch absorbs the changed failure mode. The hardcoded GUID call at legacy-comment-manager/src/main.ts:291 is commented out and intentionally out of scope.)
• Code duplication between the two factories (fixed during review: extracted ParatextProjectDataProviderFactoryBase abstract class. Shared state (_pdpMap, _creationLock, _random, _paratextProjects), StartFactoryAsync, the double-checked-locking GetProjectDataProviderID, and GetExistingProjectDataProvider now live in the base. Concrete classes provide only what actually differs: GetAvailableProjects, ShouldServeProject, GetCrossFactoryRejectionMessage, GetRegistrationTaskDescription. Concrete factories shrank from a combined ~273 lines to ~106.)
• GetAllProjectDetails is a third partition path that could drift from the two helpers the factories use (fixed during review: re-implemented as GetAvailableUnpublishedProjectDetails().Concat(GetAvailablePublishedProjectDetails()). The partition helpers are now the single source of truth — any future filtering added to either partition automatically applies to GetAllProjectDetails and to InventoryDataProvider's caller.)
• Inconsistency in how the interface list is requested between the two factories (fixed during review: both factories now use the explicit GetParatextProjectInterfaces(isPublished: false|true) form. The no-arg overload was removed entirely as part of the dead-code cleanup below.)

Minor — Consider

• ParatextPublishedProjectDataProviderFactory.GetExistingProjectDataProvider was added "for symmetry" with no current caller (fixed during review: as a side effect of the base-class refactor, GetExistingProjectDataProvider is now a single method on the abstract base — not a duplicated symmetrical hook. The YAGNI concern dissolved.)
• No-arg GetParatextProjectInterfaces() shim lacks a doc-comment / is a backward-compat-only seam (fixed during review: removed entirely. The only production caller (LoadProjectDetails) was itself dead code; the only test (GetParatextProjectInterfaces_NoArgs_MatchesUnpublished) only existed to pin the no-arg behavior and is gone too.)
• LocalParatextProjects.LoadProjectDetails (private) might unconditionally advertise the unpublished interface list (fixed during review: deleted as unused private dead code. It had no callers in production or test code.)
• Class doc on ParatextPublishedProjectDataProviderFactory uses line-number references that rot (fixed during review: replaced ParatextProjectDataProvider.cs line 654 / 660 with ParatextProjectDataProvider.VerifyUserCanCreateComments (the SBA guard) and ParatextProjectDataProvider.VerifyUserCanCreateComments (the TransliterationWithEncoder guard). Also simplified the external WordListForm.cs (lines 950, 2205) reference to just WordListForm.)
• ParatextProjectDataProvider._commentManager initialized unconditionally (fixed during review: now Lazy<CommentManager>. For published PDPs the comment wire methods are not registered, so _commentManager.Value is never accessed and CommentManager.Get is never called — eliminating the per-PDP allocation/XML-load cost on published projects. 14 call sites updated to use .Value; thread-safety is the Lazy default ExecutionAndPublication, matching the safety needs of the existing eager init.)
• Two new factory-level tests in ParatextProjectDataProviderFactoryTests were soft-coupled to the static interface list and couldn't exercise the partition (DummyScrText is always unpublished) (fixed during review: removed RegularFactory_AdvertisesLegacyCommentInterfaceAsync and RegularFactory_GetAvailableProjects_OmitsPublishedAsync. The interface-list assertion is already covered exhaustively by LocalParatextProjectsTests.GetParatextProjectInterfaces_Unpublished_IncludesLegacyComment / _Published_ExcludesLegacyCommentButKeepsScripture; the "regular factory serves an unpublished project" assertion is already covered by GetProjectDataProviderID_ReturnsIdForProviderAsync. After the removals the file's net diff against origin/ai/main is empty.)
• Factory rejection used LEGACY_COMMENT membership as discriminator instead of IsResourceProject directly (fixed during review — author catch: changed ShouldServeProject(ProjectDetails) to ShouldServeProject(ScrText) and now checks scrText.IsResourceProject directly. The base class resolves the ScrText once and derives ProjectDetails from it. This keeps the published factory's documented contract honest — future restricted-interface project types that are not resource projects (e.g. TransliterationWithEncoder if it later drops LEGACY_COMMENT) will not slip into the published factory.)

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• The class-level doc on ParatextPublishedProjectDataProviderFactory is unusually thorough — it explicitly enumerates which other PT9 restricted-comment project types are intentionally NOT served by this factory (SBA, TransliterationWithEncoder, note-only types) and explains the rationale for each, plus a forward-looking rule that future restricted patterns should get their own factory rather than be folded in.
• Comment methods are gated at registration time (ParatextProjectDataProvider.GetFunctions) using the advertised interface list rather than a second isPublished flag — this keeps the wire surface consistent with the advertised projectInterfaces as the single source of truth.
• Cross-factory rejection (both factories explicitly throw for projects belonging to the other) is a defense-in-depth choice that catches "wrong-factory" misroutes loudly instead of silently serving a PDP with mis-advertised interfaces.
• The new ParatextProjectDataProviderWireSurfaceTests are the right granularity: they verify the actual observable behavior (which method names appear on the wire) rather than mocking internals.
• The s_paratextUnpublishedProjectInterfaces rename (from s_paratextProjectInterfaces) makes the partition explicit at the data level, not just the method level.
• Exception-message patterns in the new factory mirror the existing one exactly ("Unknown project ID: ...", "Internal error adding project data provider"), so no localization regression — these are internal exception messages, not user-facing wire responses.
• The original 8 commits tell the story of the change clearly (split interface lists → add partition helpers → gate comment-method registration → add factory → wire it up). The granularity makes review tractable.
• The docs/superpowers/plans/2026-05-27-paratext-published-pdpf-split.md plan file is committed alongside the implementation, consistent with the project's "include supporting files in commits" rule.

Interview Notes

Stated purpose: split published projects into a dedicated factory so they accurately represent that they do not have comments.

Key author judgments and reasoning:

• The discriminator question: I initially flagged the cross-factory rejection logic as using an indirect property (LEGACY_COMMENT membership) but only as a minor concern. The author re-raised it as an Important issue and proposed using ScrText.IsResourceProject directly, citing the published factory's own documented contract: published-only, with explicit exclusion of TransliterationWithEncoder and other restricted-interface project types. This is a non-obvious correctness improvement that protects the factory's documented contract against future drift in the unpublished interface list. The refactor went from ShouldServeProject(ProjectDetails) to ShouldServeProject(ScrText) and the base class now resolves the ScrText once and derives ProjectDetails from it.
• On code duplication, the author confirmed they had not closely considered an abstract base class. After the refactor was proposed they asked for it to be implemented. The base class extraction was clean — concrete classes shrank from ~273 lines to ~106 — and existing tests passed without changes.
• On the wire-surface change being observable to extensions, the author asked for the non-test callers to be investigated explicitly (the test call at legacy-comment-manager/src/main.ts:291 is commented out and intentionally excluded from this PR's scope). The investigation traced all five other call sites and confirmed each one handles the absent-PDP case acceptably — either through React-level null-checks, the withPdp helper, the layering PDP factory's automatic interface-cascade, or an existing try/catch on the controller path. A pre-existing UX issue was noted (top-menu "Insert comment" / "Open Comments…" items in platform-scripture-editor are not gated by canUserCreateComments, so they appear for resource projects too) but it is NOT introduced by this PR — the same items already misbehaved before the split, just with a different error path.
• On the _commentManager initialization concern, the author left the judgement call to me. I chose Lazy<CommentManager> over making the field nullable because it preserves the existing semantics at every call site behind a .Value accessor and keeps the registration-time guard in GetFunctions as the only place that knows about the "is this PDP a published one" condition.
• On the two overlapping factory-level tests, the author left the judgement call to me. I removed them because the meaningful assertions are already covered by LocalParatextProjectsTests (interface lists, partition helpers) and ParatextProjectDataProviderFactoryTests.GetProjectDataProviderID_ReturnsIdForProviderAsync (the regular factory can serve an unpublished project). After the removals the file's net diff against the base is empty.

Unresolved items: None. Every Critical/Important finding was either dismissed with verified author reasoning or fixed during the review. The author demonstrated clear understanding throughout and did not defer to AI for any portion of the change.

In-Review Quality Check

• dotnet build (c-sharp): clean (0 errors; pre-existing warnings unrelated to this PR).
• dotnet test --filter "FullyQualifiedName~Projects": 217/217 passing (was 220 before the two-overlap-test removal and one no-arg-shim-test removal — net minus 3 is expected).
• dotnet csharpier .: clean (no reformatting needed on this PR's files).
• npm run typecheck: passes cleanly.
• npm run lint: passes cleanly (2 pre-existing no-console warnings in extensions/src/platform-scripture/src/use-effective-resource-reference-list.ts are unrelated to this PR).
• npm run format: clean — no formatting changes applied.
• npm test: all non-React workspaces pass (paranext-core, eslint-plugin-paranext, papi-dts, platform-bible-utils, and the extension suites). The platform-bible-react Storybook test worker crashed mid-run (42 of 93 files reported as passed before the worker died). The crash is a pre-existing flake in the Storybook component runner and has zero relationship to this PR (no TypeScript / React files were modified — all changes are C#).

Suggested Review Focus

Prioritized areas for the author-reviewer meeting:

• Wire-surface change — confirm with the human reviewer that no other external callers of legacyCommentManager.comments were missed. The five non-test callers in this repo were traced (comment-list.web-view.tsx, platform-scripture-editor.web-view.tsx:357, platform-scripture-editor/main.ts:548, the USJ layering PDPF, and the editor's USJ web-view reference). All handle the absent-PDP case acceptably.
• Pre-existing UX gap (not in this PR) — the editor's top-menu items "Insert comment" and "Open Comments…" are unconditionally present even on resource projects. Pre-existing behavior; this PR makes the failure path slightly more explicit (factory rejection instead of permission denied), but the visible behavior is essentially the same. Worth noting for a follow-up.
• ShouldServeProject discriminator choice — the move from LEGACY_COMMENT membership to ScrText.IsResourceProject was the author's catch and protects the published factory's documented contract. Worth a quick read-through of the base class to confirm the ScrText is resolved exactly once and ProjectDetails is still derived from the same ScrText.
• _commentManager laziness — confirm that thread-safety semantics of Lazy<CommentManager> (default ExecutionAndPublication) match what the rest of the PDP expects under concurrent comment-method invocation.
• Class-level doc maintenance — the ParatextPublishedProjectDataProviderFactory doc-comment is now a load-bearing decision record (which project types are/aren't served and why). Worth keeping in mind that future restricted-interface project types should update it explicitly rather than being silently rolled in.

---

This change is 

[captaincrazybro]

@captaincrazybro made 5 comments.
Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions (waiting on tjcouch-sil).

---

c-sharp/Projects/LocalParatextProjects.cs line 123 at r1 (raw file):

    /// Used by <see cref="ParatextProjectDataProviderFactory"/> to populate its project list.
    /// </summary>
    public IEnumerable<ProjectDetails> GetAvailableUnpublishedProjectDetails()

This function and the one bellow seem significantly similar and I would think you could still have one GetAvailableProjectDetails() with a parameter similar to the one below which is bool isPublished.

---

c-sharp/Projects/LocalParatextProjects.cs line 164 at r1 (raw file):

    }

    private static ProjectDetails? LoadProjectDetails(

Was this function just completely unused? Curious to why this was removed.

---

c-sharp/Projects/ParatextProjectDataProviderFactory.cs line 18 at r1 (raw file):

            papiClient,
            paratextProjects,
            LocalParatextProjects.GetParatextProjectInterfaces(isPublished: false),

Why does this need isPublished: false. Can't it just be GetParatextProjectInterfaces(false)? Similar with other places...

---

c-sharp/Projects/ParatextProjectDataProviderFactory.cs line 22 at r1 (raw file):

        ) { }

    protected override List<ProjectMetadata>? GetAvailableProjects(JsonElement _ignore)

Should these functions have summary comments above them like the base function bellow?

---

c-sharp/Projects/ParatextProjectDataProviderFactoryBase.cs line 75 at r1 (raw file):

    );

    public override string GetProjectDataProviderID(string projectID)

Should this have a summary comment?

[captaincrazybro]

Also do any changes need to be made for the frontend after this PR?

@captaincrazybro made 1 comment.
Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions (waiting on tjcouch-sil).

[lyonsil]

One non-blocking comment

@lyonsil reviewed 11 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on captaincrazybro and tjcouch-sil).

---

c-sharp/Projects/LocalParatextProjects.cs line 164 at r1 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Was this function just completely unused? Curious to why this was removed.

I was going to add a similar comment, then I searched for "LoadProject" in the PR details and saw it was intentional. Then I looked through the project history to understand what its original purpose was. This was used when we tried to load projects directly from the "My Paratext 9 Projects" folder in Windows when on the C drive. We have since abandoned that approach, and I don't see it coming back anytime soon. So deleting makes sense to me.

---

c-sharp-tests/Projects/ParatextProjectDataProviderWireSurfaceTests.cs line 9 at r1 (raw file):

    internal class ParatextProjectDataProviderWireSurfaceTests : PapiTestBase
    {
        private static readonly string[] AllCommentWireMethodSuffixes =

This test seems fragile if we add more comment methods. It would be nicer if this could pull all the comment method names from somewhere instead of hard coding it here.