PT-3614: added unimplemented placeholder command to commit changes for version history

[captaincrazybro]

This is a small PR that implements a placeholder command handler (throws that it's not implemented) to commit changes for the version history to be implemented in the paratext-10-studio repo patches.

The commands were then implemented in the following places:

• Before and after a footnote/cross-reference is inserted (both are force commits)
• Does a daily commit when the editor saves the project triggered when any change is made
• Before and after a single replace action is done (first is a force commit, the second is not)
• Before and after a replace all action is done (first is a force commit, the second is not)

P.S. I scoured the Paratext 9 codebase and the .md file that was attached to the PT-3614 ticket that Matt made to find all the usages of the commit commands that were in Paratext 9 for features that had only been already implemented in Paratext 10. And there didn't seem to be that many places that they should be implemented. Please correct me if there are any places I missed!

---

This change is 

---

[katherinejensen00]

This looks great. Thanks for making these changes, Levi! This will be a nice feature to have.

@katherinejensen00 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved.

[katherinejensen00]

@katherinejensen00 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved.

[tjcouch-sil]

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

---

c-sharp/Services/VersionHistoryService.cs line 10 at r2 (raw file):

/// Service that manages version history for the projects
/// </summary>
internal class VersionHistoryService(PapiClient papiClient)

I believe this needs to be c-sharp/Projects/SendReceive/ParatextProjectSendReceiveService.cs to properly make a placeholder file for the service in the P10 patch.

---

c-sharp/Services/VersionHistoryService.cs line 16 at r2 (raw file):

    public async Task InitializeAsync()
    {
        // Set up commands on the PAPI

These commands need to be put in an extension .d.ts file so they are visible on the PAPI. Maybe you can put them in extensions/src/platform-get-resources/src/types/platform-get-resources.d.ts. But they should have the normal paratextBibleSendReceive. prefix like the others. I know it's a bit weird, but this is what I've got in mind for now :)

---

c-sharp/Services/VersionHistoryService.cs line 36 at r2 (raw file):

    /// </summary>
    /// <returns>Whether there were changes to commit (if not forcing)</returns>
    private Boolean Commit(String projectId, String comment, Boolean forceCommit)

In what situation would we want to force a commit? Seems a little risky

---

c-sharp/Services/VersionHistoryService.cs line 38 at r2 (raw file):

    private Boolean Commit(String projectId, String comment, Boolean forceCommit)
    {
        throw new Exception("This command is unimplemented!");

Maybe the exception message should be something more like "Must be running Paratext 10 Studio to use this command" for all of these. Also can be pulled out to a shared const string.

---

c-sharp/Services/VersionHistoryService.cs line 50 at r2 (raw file):

    /// <summary>
    /// Function that quickly commits changes, bypassing merger check and few other things

What is the meaningful difference between this and Commit? In what use cases would you use one or the other? I'm not sure we want something that skips checks; maybe remove this after discussing.

[tjcouch-sil]

Thanks for the hard work!

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

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 16 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

These commands need to be put in an extension .d.ts file so they are visible on the PAPI. Maybe you can put them in extensions/src/platform-get-resources/src/types/platform-get-resources.d.ts. But they should have the normal paratextBibleSendReceive. prefix like the others. I know it's a bit weird, but this is what I've got in mind for now :)

Will do

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 36 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

In what situation would we want to force a commit? Seems a little risky

Check the SavePoint triggers file Matt made

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 38 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Maybe the exception message should be something more like "Must be running Paratext 10 Studio to use this command" for all of these. Also can be pulled out to a shared const string.

I was just following the proposed guidelines from the ticket. Does this encompass all use cases though? What if the user/developer has no plan of using Paratext 10 Studio. They need to know that it simply unimplemented.

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 50 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

What is the meaningful difference between this and Commit? In what use cases would you use one or the other? I'm not sure we want something that skips checks; maybe remove this after discussing.

Check the SavePoint triggers file Matt made

[tjcouch-sil]

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

---

c-sharp/Services/VersionHistoryService.cs line 36 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Check the SavePoint triggers file Matt made

Ah I see; if the doc is to be trusted, basically "force" just means "make a revision even if there isn't anything changed". That's fine. Please add documentation comments explaining what this parameter does.

---

c-sharp/Services/VersionHistoryService.cs line 38 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

I was just following the proposed guidelines from the ticket. Does this encompass all use cases though? What if the user/developer has no plan of using Paratext 10 Studio. They need to know that it simply unimplemented.

You can say "This command is unimplemented in Platform.Bible. Must be running Paratext 10 Studio to use this command" if you want. Though, as of recent product-level decisions above my head, we aren't currently intending for anyone to use only Platform.Bible; the product is Paratext 10 Studio, and Platform.Bible is kinda an implementation detail these days.

---

c-sharp/Services/VersionHistoryService.cs line 50 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Check the SavePoint triggers file Matt made

Looks like that file says QuickCommit is only used in PT Live. Unless you know something else, please remove this. Seems like it's prone to causing problems if not used correctly. What can go wrong will go wrong ;) if you know more that might change this thought process, let me know.

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 36 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Ah I see; if the doc is to be trusted, basically "force" just means "make a revision even if there isn't anything changed". That's fine. Please add documentation comments explaining what this parameter does.

Done.

[captaincrazybro]

c-sharp/Services/VersionHistoryService.cs line 50 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Looks like that file says QuickCommit is only used in PT Live. Unless you know something else, please remove this. Seems like it's prone to causing problems if not used correctly. What can go wrong will go wrong ;) if you know more that might change this thought process, let me know.

Will remove

[captaincrazybro]

@captaincrazybro made 4 comments.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on katherinejensen00 and tjcouch-sil).

---

c-sharp/Services/VersionHistoryService.cs line 10 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I believe this needs to be c-sharp/Projects/SendReceive/ParatextProjectSendReceiveService.cs to properly make a placeholder file for the service in the P10 patch.

Done.

---

c-sharp/Services/VersionHistoryService.cs line 16 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Will do

Done.

---

c-sharp/Services/VersionHistoryService.cs line 38 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

You can say "This command is unimplemented in Platform.Bible. Must be running Paratext 10 Studio to use this command" if you want. Though, as of recent product-level decisions above my head, we aren't currently intending for anyone to use only Platform.Bible; the product is Paratext 10 Studio, and Platform.Bible is kinda an implementation detail these days.

Done.

---

c-sharp/Services/VersionHistoryService.cs line 50 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Will remove

Done.

[katherinejensen00]

@katherinejensen00 reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on captaincrazybro and tjcouch-sil).

---

c-sharp/Projects/SendReceive/ParatextProjectSendReceiveService.cs line 38 at r6 (raw file):

    /// </summary>
    /// <returns>Whether there were changes to commit (if not forcing)</returns>
    protected Boolean CommitChanges(String projectId, String comment, Boolean forceCommit = false)

Minor I may be missing something, but saving a snapshot of history seems like something we would do in more contexts (e.g. Replace All) than just Send/Receive. Putting it here probably isn't wrong, just a little bit confusing. I see that TJ suggested it though so it is probably right, just trying to understand it.

[captaincrazybro]

c-sharp/Projects/SendReceive/ParatextProjectSendReceiveService.cs line 38 at r6 (raw file):

Previously, katherinejensen00 wrote…

Minor I may be missing something, but saving a snapshot of history seems like something we would do in more contexts (e.g. Replace All) than just Send/Receive. Putting it here probably isn't wrong, just a little bit confusing. I see that TJ suggested it though so it is probably right, just trying to understand it.

That's the other half of the ticket that I just implemented. As it is this command does nothing unless implemented.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 15 additional findings in Devin Review.

[devin-ai-integration]

🟡 isInsertingFootnote/isInsertingCrossReference refs can become permanently stale, causing wrong commit type on subsequent saves

The isInsertingFootnote and isInsertingCrossReference refs are set to true at platform-scripture-editor.web-view.tsx:706 and platform-scripture-editor.web-view.tsx:727 after insertMarker is called, and they are only cleared inside saveUsjToPdpInternal on a successful save (platform-scripture-editor.web-view.tsx:1159-1170). If insertMarker doesn't produce a USJ change (e.g., no valid selection, cursor in invalid position) or the save fails, the flag persists indefinitely. On the next successful save — which could be a completely unrelated regular text edit — the code incorrectly enters the footnote/cross-reference commit branch instead of calling commitDaily, producing a misleading version history entry.

Example stale flag scenario

1. User triggers insert footnote, but insertMarker('f') silently fails (no USJ change).
2. isInsertingFootnote.current remains true.
3. User makes a normal text edit → save succeeds → code hits if (isInsertingFootnote.current) and sends a "After inserting footnote" commit for an unrelated edit.

Similarly, if both flags get set to true (insert footnote fails silently, then insert cross-reference fails silently), the if/else-if structure at line 1159 means only isInsertingFootnote is consumed first, and isInsertingCrossReference leaks to a later save.

Prompt for agents

The isInsertingFootnote and isInsertingCrossReference refs in platform-scripture-editor.web-view.tsx are set unconditionally after insertMarker, but they are only cleared when a successful save occurs in saveUsjToPdpInternal. If insertMarker does not produce a USJ change (no onUsjChange fires), the flag persists and is incorrectly consumed by the next save.

Possible approaches:
1. Set the flag BEFORE calling insertMarker, then check after insertMarker whether the USJ actually changed. If it didnt change, immediately clear the flag.
2. Add a timeout or guard that clears the flag if no save happens within a reasonable time after setting it.
3. Use a more explicit state machine rather than bare boolean refs, where the state includes a timestamp or save-generation counter, so a stale flag from a previous operation can be detected and ignored.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[katherinejensen00]

@katherinejensen00 reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on captaincrazybro and tjcouch-sil).

[tjcouch-sil]

Very nice improvements! Thanks for the hard work. Just a couple things left to look into. Also, when you get ready, would you mind running /review-paratext on it please? Thanks!

@tjcouch-sil reviewed 4 files, made 4 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on captaincrazybro).

---

extensions/src/platform-scripture/src/find.web-view.tsx line 931 at r15 (raw file):
In my message about what to do, I put the following:

Make sure to handle gracefully if it throws (if the error is the unimplemented error, keep going; maybe make a debug or info log or something. Otherwise, throw so it doesn't continue with whatever it was trying to do). This should not just be in patches in P10; hopefully, most of the code will be in P.B or such.

If these "commit" commands are unimplemented, we need to do an info log or something and keep going. But if the error is not just that this is unimplemented, this means something really went wrong, and we shouldn't continue with the replace operation (the other scenarios like adding a footnote are just normal editing, so I don't guess we should prevent the operation in those cases; just log info if it's unimplemented or warning if it is a real error) for fear of really messing someone's project up.

In the past, we have detected specific errors thrown with specific exceptions in C# like MissingBookException.cs and detecting the exact text where it might be thrown. Could you investigate and try to implement this kind of thing for this situation?

---

c-sharp/Services/VersionHistoryService.cs line 38 at r2 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

Done.

(Note you're blocking on this thread for some reason though I have set my status to resolved)

---

extensions/src/platform-scripture/src/find.web-view.tsx line 926 at r15 (raw file):

              'paratextBibleSendReceive.commitChanges',
              projectId,
              `${localizedStrings['%versionHistoryCommit_beforeReplace%']}: <vern>${searchTerm}\u2014>${replaceTerm}</vern>`,

I suspect all these commit strings need to follow the Concatenate strings with format strings rule.

For example, here, you'd pretty much just want the Before doing replace: part to be localizable; the rest should be a format string.

Do you know if Paratext 9 relies on these comments to be in a specific format in order for it to read them properly? I'm guessing not, but P9 never ceases to surprise me. I'm sure the <vern> stuff is all specific, so you don't want to put that in the localizable part of the string, but I'm guessing it doesn't matter in what part of the string that goes.

But this might be a case where it makes more sense not to localize the string concatenation... It's hard to say. Maybe Todd would know?

[captaincrazybro]

extensions/src/platform-scripture/src/find.web-view.tsx line 926 at r15 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I suspect all these commit strings need to follow the Concatenate strings with format strings rule.

For example, here, you'd pretty much just want the Before doing replace: part to be localizable; the rest should be a format string.

Do you know if Paratext 9 relies on these comments to be in a specific format in order for it to read them properly? I'm guessing not, but P9 never ceases to surprise me. I'm sure the <vern> stuff is all specific, so you don't want to put that in the localizable part of the string, but I'm guessing it doesn't matter in what part of the string that goes.

But this might be a case where it makes more sense not to localize the string concatenation... It's hard to say. Maybe Todd would know?

So, the part after the localized strings as I understand is probably markdown/formatting. The way the string is formatted above is exactly how Paratext 9 did it. I'll adjust the localized strings though.

[captaincrazybro]

extensions/src/platform-scripture/src/find.web-view.tsx line 931 at r15 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

In my message about what to do, I put the following:

Make sure to handle gracefully if it throws (if the error is the unimplemented error, keep going; maybe make a debug or info log or something. Otherwise, throw so it doesn't continue with whatever it was trying to do). This should not just be in patches in P10; hopefully, most of the code will be in P.B or such.

If these "commit" commands are unimplemented, we need to do an info log or something and keep going. But if the error is not just that this is unimplemented, this means something really went wrong, and we shouldn't continue with the replace operation (the other scenarios like adding a footnote are just normal editing, so I don't guess we should prevent the operation in those cases; just log info if it's unimplemented or warning if it is a real error) for fear of really messing someone's project up.

In the past, we have detected specific errors thrown with specific exceptions in C# like MissingBookException.cs and detecting the exact text where it might be thrown. Could you investigate and try to implement this kind of thing for this situation?

Honestly, I would argue that if there is a problem with the commit commands it shouldn't stop the replace operation. Currently they don't need the commit commands to operate as it is, and I don't think the entire replace operation should fail if there is a single commit failure.

[captaincrazybro]

extensions/src/platform-scripture/src/find.web-view.tsx line 926 at r15 (raw file):

Previously, captaincrazybro (Cqptain) wrote…

So, the part after the localized strings as I understand is probably markdown/formatting. The way the string is formatted above is exactly how Paratext 9 did it. I'll adjust the localized strings though.

Done.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 16 additional findings in Devin Review.

[devin-ai-integration]

🔴 English localized strings missing for version history commit messages

The four new localization keys (%versionHistoryCommit_beforeInsertFootnote%, %versionHistoryCommit_afterInsertFootnote%, %versionHistoryCommit_beforeInsertCrossReference%, %versionHistoryCommit_afterInsertCrossReference%) were added to the Spanish ("es") section at extensions/src/platform-scripture-editor/contributions/localizedStrings.json:128-131 but are completely missing from the English ("en") section (extensions/src/platform-scripture-editor/contributions/localizedStrings.json:17-89). The localization service returns the raw key name when no localization is found (src/extension-host/services/localization.service-host.ts:330), so English users will see commit messages like %versionHistoryCommit_beforeInsertFootnote% instead of "Before inserting footnote".

Prompt for agents

The four new versionHistoryCommit localization keys were added only to the Spanish (es) section of extensions/src/platform-scripture-editor/contributions/localizedStrings.json but are missing from the English (en) section. Add these entries to the en section (around line 54, after the existing entries and before webView_footnoteList_header):

%versionHistoryCommit_afterInsertCrossReference%: After inserting cross reference
%versionHistoryCommit_afterInsertFootnote%: After inserting footnote
%versionHistoryCommit_beforeInsertCrossReference%: Before inserting cross reference
%versionHistoryCommit_beforeInsertFootnote%: Before inserting footnote

Without these, English users will see raw localize keys as commit messages.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 18 additional findings in Devin Review.

[devin-ai-integration]

🟡 finally block clears insertion-tracking refs before retry save completes

In saveUsjToPdpInternal, when the PDP save returns falsy (!saveResult), the else if branch resets currentlyWritingUsjToPdp and calls saveUsjToPdpIfUpdatedInternal(editorUsj) which starts a new async saveUsjToPdpInternal (not awaited — fire-and-forget at platform-scripture-editor.web-view.tsx:1138). The second call runs synchronously until its first await (line 1154), then yields. After it yields, the first call's finally block (lines 1223-1226) executes and clears isInsertingFootnote.current / isInsertingCrossReference.current. When the second call's await later resolves, the flags are already false, so it falls into the commitDaily branch (line 1178) instead of the correct commitChanges with the after-insert message.

Prompt for agents

The finally block in saveUsjToPdpInternal (lines 1223-1226) unconditionally clears isInsertingFootnote and isInsertingCrossReference refs. However, when a save retry is initiated from the !saveResult path (line 1193 calls saveUsjToPdpIfUpdatedInternal which fire-and-forgets a new saveUsjToPdpInternal), the finally block of the first invocation clears the flags before the second invocation's await resolves.

Possible approaches:
1. Move the flag-clearing logic out of the finally block and into each specific branch where it's safe (success path already clears them at lines 1160/1169; the catch path at line 1198 could clear them too; remove them from finally).
2. Only clear the flags in finally if currentlyWritingUsjToPdp.current is false (indicating no retry was started).
3. Copy the flag values into local variables at the start of saveUsjToPdpInternal and use those locals instead of the refs for the commit logic, so the finally block clearing the refs doesn't affect the retry.

Relevant functions: saveUsjToPdpInternal and saveUsjToPdpIfUpdatedInternal in platform-scripture-editor.web-view.tsx around lines 1132-1228.

---

Was this helpful? React with 👍 or 👎 to provide feedback.