Skip to content

Conversation

pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Sep 24, 2025

I have decided to separate the backend change from the frontend change, in the hope of reducing the complexity of code review.

I have marked this PR as testing not required, as it is backend only, but if you want to test the change yourself, use the branch feature/SF-3567-frontend, which I have wired up to test this code (in a very basic manner).

Also included is a fix for oversight where ReplaceDoc support did not work for Deltas. I was originally going to use ReplaceDoc() until I hit this bug, implemented support in the realtime server, then decided not to go that route, but thought that I might as well retain the fix for the realtime server.

This change is Reviewable

Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 92.05955% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.31%. Comparing base (02f8060) to head (d20477d).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs 91.66% 21 Missing and 9 partials ⚠️
...rc/SIL.XForge.Scripture/Services/DeltaUsxMapper.cs 80.00% 0 Missing and 1 partial ⚠️
...c/SIL.XForge.Scripture/Services/NotificationHub.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3463      +/-   ##
==========================================
+ Coverage   82.20%   82.31%   +0.10%     
==========================================
  Files         611      613       +2     
  Lines       36433    36830     +397     
  Branches     6004     6043      +39     
==========================================
+ Hits        29951    30316     +365     
- Misses       5608     5643      +35     
+ Partials      874      871       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pmachapman pmachapman marked this pull request as ready for review September 24, 2025 05:29
Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmachapman dismissed @github-advanced-security[bot] from 3 discussions.
Reviewable status: 0 of 19 files reviewed, all discussions resolved

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed; have a few comments so far.

@Nateowami reviewed 1 of 19 files at r1, 17 of 18 files at r2, 13 of 18 files at r3.
Reviewable status: 14 of 19 files reviewed, 6 unresolved discussions (waiting on @pmachapman)


src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 42 at r3 (raw file):

        string scriptureRange,
        string targetProjectId,
        DateTime timestamp

Why is the timestamp required? Shouldn't "latest" be the default?


src/SIL.XForge.Scripture/Services/DeltaUsxMapper.cs line 180 at r3 (raw file):

    public static string ExtractBookId(string usfm)
    {
        string firstLine = usfm.Split('\n').FirstOrDefault()?.Trim() ?? string.Empty;

Isn't splitting the string allocating a fairly large string array? In my testing there is a project where the Psalms alone is 5MB. Of course that's an outlier, but there are 30 projects where the Psalms USFM file is at least one million bytes.

Maybe something like:

        int end = usfm.IndexOfAny(['\r', '\n']);
        string firstLine = end >= 0 ? usfm[..end] : usfm;
        var match = BookIdRegex().Match(firstLine);
        return match.Success ? match.Groups[1].Value : string.Empty;

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 33 at r3 (raw file):

using TextInfo = SIL.XForge.Scripture.Models.TextInfo;

#pragma warning disable CA2254

I don't think this is actually needed? And if it is it would be good to have a comment saying what warning it pertains to and why it needs to be turned off.


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 153 at r3 (raw file):

                await hubContext.NotifyDraftApplyProgress(
                    sfProjectId,
                    new DraftApplyState { State = $"Retrieving draft for {Canon.BookIdToEnglishName(book)}." }

Shouldn't we be setting machine-readable values that can be parsed and then displayed however the client chooses (e.g. progress bar, or localized messages)?


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 170 at r3 (raw file):

                }

                // Ensure that if chapters is blank, it contains every chapter in the book

What does this mean? Is this handling a versification mismatch?


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 179 at r3 (raw file):

                // Store the USJ for each chapter, so if we download form Serval we only do it once per book
                List<Usj> chapterUsj = [];
                foreach (int chapterNum in chapters.Where(c => c > 0))

Is a chapter zero conceptually invalid?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants