Skip to content

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Oct 16, 2025

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

❌ Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.42%. Comparing base (5c09f90) to head (19da520).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../app/serval-administration/draft-jobs.component.ts 0.00% 35 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3520      +/-   ##
==========================================
+ Coverage   82.21%   82.42%   +0.21%     
==========================================
  Files         615      615              
  Lines       37031    36936      -95     
  Branches     6068     6015      -53     
==========================================
  Hits        30444    30444              
+ Misses       5690     5608      -82     
+ Partials      897      884      -13     

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

Copy link
Collaborator Author

@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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts line 218 at r1 (raw file):

        event.eventType === 'BuildProjectAsync' ||
        event.eventType === 'RetrievePreTranslationStatusAsync' ||
        event.eventType === 'ExecuteWebhookAsync' ||

This event was originally not included as a completion event because I was looking at local event metrics to develop the feature, and the webhook is not run locally.


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts line 224 at r1 (raw file):

    const jobs: DraftJob[] = [];

    // Step 1: Find all build events (BuildProjectAsync) - these are our anchors

The changes in this file are basically throwing out the old approach to event correlation, and replacing it with the follwing:

  1. Start with BuildProjectAsync events (where we call Serval to start the build). These have a build ID.
  2. Find the StartPreTranslationBuildAsync event that immediately precedes that BuildProjectAsync event. These don't have build IDs.
  3. Find the first event that indicates a completion, which has the same build ID as BuildProjectAsync

This fixes a lot of previous issues, including where the old logic was finding the last completion event. However,if a build never reaches the stage of starting it on Serval it isn't in the list.

There's probably a lot to improve upon, but I think this is a good start.

@pmachapman pmachapman self-assigned this Oct 19, 2025
@pmachapman pmachapman force-pushed the fix/event-corelation branch from faa48c0 to 19da520 Compare October 19, 2025 18:33
Copy link
Collaborator

@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.

:lgtm: I like how your logic is a lot more straightforward. It appeared to show the same results for my builds locally as master, and I think your changes will improve the results for the cases you documented in your GitHub comment.

@pmachapman reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

@pmachapman pmachapman merged commit b0eb5a1 into master Oct 19, 2025
23 checks passed
@pmachapman pmachapman deleted the fix/event-corelation branch October 19, 2025 19:38
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