SF-3736 Use Serval data to populate builds list

[marksvc]

This introduces the Serval builds tab without yet removing the Draft jobs tab.

---

Currently, the Serval Administration area has a Draft Jobs tab which shows a list of Serval builds. It does this by fetching and correlating EventMetric records on the frontend. More information for a build is then obtained by looking up Serval build information, user information, etc.

This PR is a step toward replacing the functionality of the Draft Jobs tab. This PR adds a Serval Builds tab which shows a list of Serval builds. It does this by fetching Serval build data on the backend, and augmenting the data on the backend with details from EventMetric records and project and user metadata. The information is received by the frontend to render to the user.

The Serval Builds tab is designed to be a suitable replacement for the Draft Jobs tab. However, the Draft Jobs tab is still present so the Serval Builds tab can get some vetting first.

This change is 

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 6 additional findings.

[codecov]

Codecov Report

❌ Patch coverage is 69.80146% with 289 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (9071f22) to head (ac2cf15).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...p/serval-administration/serval-builds.component.ts	42.85%	140 Missing and 4 partials ⚠️
...SIL.XForge.Scripture/Services/MachineApiService.cs	82.89%	53 Missing and 19 partials ⚠️
...orge.Scripture/Controllers/MachineApiController.cs	0.00%	22 Missing ⚠️
.../app/serval-administration/draft-jobs.component.ts	10.52%	17 Missing ⚠️
.../serval-administration/serval-builds-statistics.ts	90.65%	3 Missing and 7 partials ⚠️
...c/app/serval-administration/serval-build-report.ts	81.57%	7 Missing ⚠️
...slate/draft-generation/draft-generation.service.ts	46.15%	7 Missing ⚠️
...serval-administration/draft-jobs-export.service.ts	60.00%	4 Missing ⚠️
...ure/Services/MachineServiceCollectionExtensions.cs	42.85%	4 Missing ⚠️
...p/src/app/machine-api/remote-translation-engine.ts	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3719      +/-   ##
==========================================
- Coverage   81.30%   81.05%   -0.26%     
==========================================
  Files         622      628       +6     
  Lines       39387    40308     +921     
  Branches     6390     6570     +180     
==========================================
+ Hits        32025    32671     +646     
- Misses       6377     6623     +246     
- Partials      985     1014      +29     

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

[Nateowami]

So is this a newer version of the draft jobs tab?

[marksvc]

So is this a newer version of the draft jobs tab?

Yes, though I will want its tires kicked and to hear feedback first.

I also haven't yet wired up the Projects tab to link over to the Serval Builds tab.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 15 additional findings in Devin Review.

[devin-ai-integration]

🟡 Events with null request IDs can produce duplicate reports for builds already matched by Serval

In GetBuildsSinceAsync at MachineApiService.cs:1190-1202, the unmatchedEventsByProject filtering uses the condition requestId == null || !matchedRequestIds.Contains(requestId). Events where GetRequestIdFromEvent returns null always pass the filter, even if they were already correlated to a Serval build by FindEventTime (which matches on e.Result?.ToString() == servalBuildId). This means a StartPreTranslationBuildAsync event that was already used to populate sfUserRequested in a Serval-build-based report will also generate a separate events-only report via BuildEventsOnlyReports at line 1664-1690.

This primarily affects older data (pre-2026) that lacks draft generation request ID tags. The result is duplicate entries in the builds list for the same underlying draft generation request.

Prompt for agents

In MachineApiService.cs GetBuildsSinceAsync method, around line 1168-1184, in addition to tracking matchedRequestIds, also track the set of event metric IDs (or timestamps+eventTypes) that were successfully correlated to Serval builds during BuildReportAsync/BuildTimeline processing. Then in the unmatchedEventsByProject filtering at line 1190-1202, exclude events that were already used to build timeline data for a Serval build report, even if they have a null request ID. One approach is to collect all event IDs that were matched during FindEventTime calls and filter them out when building unmatchedEventsByProject.

---

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

[RaymondLuong3]

It would have been helpful to make this PR in stages. There is so much in here that it was not easy to know what to look for in reviewing it. But it is nice to see the full component be fully functional.
I didn't carefully review all the code but since it is a component mostly for serval admins I didn't think I needed to be too careful. Small adjustments can be made afterwards. Nice work!

@RaymondLuong3 partially reviewed 47 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.ts line 23 at r4 (raw file):

  requesterSFUserId: string | undefined;
  /** Status of the build. */
  status: DraftGenerationBuildStatus;

Nit: Some of these comments aren't really helpful

Code quote:

  /** The SF user who requested this build, if known. */
  requesterSFUserId: string | undefined;
  /** Status of the build. */
  status: DraftGenerationBuildStatus;

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.ts line 159 at r4 (raw file):

 * ranges into individual book identifiers.
 */
export function toProjectBooks(ranges: BuildReportProjectScriptureRange[] | undefined): ProjectBooks[] {

We have a function booksFromScriptureRange() to take a scripture range and return the books in utils.ts Could we reuse that here?

Code quote:

export function toProjectBooks(ranges: BuildReportProjectScriptureRange[] | undefined): ProjectBooks[] {

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.spec.ts line 8 at r4 (raw file):

} from './serval-build-report';

describe('toProjectBooks', () => {

These tests wouldn't be needed if using booksFromScriptureRange function that we already implement.

Code quote:

describe('toProjectBooks', () => {

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds-statistics.ts line 265 at r4 (raw file):

    totalBuilds: totalBuilds,
    totalProjects: totalProjects,
    buildsPerProjectRatio: buildsPerProjectRatio,

Nit: This seems to be redundant, but also a result of the instructions Agent.md Is there a reason you like this verbosity?

Code quote:

    totalBuilds: totalBuilds,
    totalProjects: totalProjects,
    buildsPerProjectRatio: buildsPerProjectRatio,

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.html line 281 at r4 (raw file):

                    <mat-card-title-group>
                      <mat-icon>merge</mat-icon>
                      <mat-card-title>Training</mat-card-title>

Nit: This card includes training and translation. Maybe a better title would be "Language Model" or "Input/Output"

Code quote:

<mat-card-title>Training</mat-card-title>

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.scss line 384 at r4 (raw file):

}

.timing-bar {

Nit: I don't think this timing bar is really necessary.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 737 at r4 (raw file):

  /** Formats a language code with its display name, e.g. "es (Spanish)". Falls back to just the code. */
  protected formatLanguageWithName(langCode: string | undefined): string {

You can reuse i18n.service.ts which has getLanguageDisplayName() for this purpose.

Code quote:

  protected formatLanguageWithName(langCode: string | undefined): string {

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/copy/copy.component.ts line 24 at r4 (raw file):

  @Input() iconName: string = 'content_copy';
  @Input() copiedIconName: string = 'done';
  @Input() copiedDurationMs: number = 1000;

Nit: I would set this longer to 5s. 1s seems too short to me.

Code quote:

  @Input() copiedDurationMs: number = 1000;

[marksvc]

Thank you. And thank you for your effort in looking over this huge PR!

I hear you on it being easier to review in chunks/stages.

In development I needed to go back and revise the backend as I learned more and implemented more of the frontend. Perhaps I could have made different PRs for the backend code vs the frontend code once I had it finalized. On the other hand it is also nice to be able to present the code as a cohesive working set of changes. Hmm.

Thank you for your perseverance.

By the way, I find that Devin Review helpfully breaks down and groups changes so it's easier to review something big. So that is something that could be helpful in this situation.

@marksvc made 9 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.ts line 23 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: Some of these comments aren't really helpful

I had doubled up comments/documentation between TS and C# files, and have deleted the duplicate comments on TS fields. (Otherwise they can get out of sync.). I left the field comments in the C# files.

I left TS class/interface comments.

Hmm. I'm reluctant to remove these last two field comments from the C# file. I think "Status of the build" communicates more than Status, for example. I could probably be convinced tho :)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.ts line 159 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

We have a function booksFromScriptureRange() to take a scripture range and return the books in utils.ts Could we reuse that here?

Done. And this brought to my attention the per-chapter reporting that we can now do. I made a note to pass through and display those in a future change.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.spec.ts line 8 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

These tests wouldn't be needed if using booksFromScriptureRange function that we already implement.

Thank you. The toProjectBooks function does additional processing related to SF project IDs, project names, and returns a ProjectBooks type with a different structure of information than booksFromScriptureRange. Let's chat some more if that isn't convincing.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds-statistics.ts line 265 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This seems to be redundant, but also a result of the instructions Agent.md Is there a reason you like this verbosity?

Thank you. I don't think we need "Ratio" there and have removed it.

Though I'm not sure if that's what you mean :). I computed a set of info on the currently shown table of builds that I thought might be informative.
And I also wasn't shy in naming the variables in descriptive ways.
Is one of these others what you are meaning by verbosity or redundancy?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.html line 281 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This card includes training and translation. Maybe a better title would be "Language Model" or "Input/Output"

You're right. And looking at this more I also see how it would make sense to combine some of this information with the information in the "Language info" card. Considering the information presented on this card, I think "Input" may be a good name. All the training source+target material, and the translation source material are all inputs.

What do you think of this?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.scss line 384 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: I don't think this timing bar is really necessary.

Thank you. The visual is superfluous if one knows how to interpret each duration. My concern is that it may not be clear when a listed duration ends. I am so far listing 4 durations on the Timing card. And they all overlap with two or more of each other. One duration goes for the entire length; one goes for just the Serval parts, and two are just for their own phase. And I am concerned that there could be confusion about what range of activities is spanned by a duration.

Unfortunately the bars doesn't show up well on dev machines because of how they interact with Serval, so it may not have been clear what these bars are doing. When running on QA or Live the visuals should look more like the image below, where we can see an outer duration bar showing the beginning and end of the whole process, and an inner duration bar showing the start and end of the Serval activity.

There are no doubt many ways we could communicate the overlapping duration information. I am open to better ideas :D

(Timing information in the screenshot is very fake; just getting something together.)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 737 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

You can reuse i18n.service.ts which has getLanguageDisplayName() for this purpose.

Ah, thank you for that! Done.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/copy/copy.component.ts line 24 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: I would set this longer to 5s. 1s seems too short to me.

Okay. I've found other examples in the wild to be shorter times[1], but we can increase it and keep an ear open for preferences for longer or shorter. Done.

[1] eg each of these seems to be about 1.5 s:

• https://developer.hashicorp.com/vagrant/install
• https://brave.com/linux/
• https://flathub.org/en/setup/Fedora

[devin-ai-integration]

Devin Review found 6 new potential issues.

View 11 additional findings in Devin Review.

[devin-ai-integration]

🟡 Object property shorthand used in new code violates AGENTS.md rule

The AGENTS.md rule states: "Do not use object property shorthand when creating objects. For example, do not write const obj = { foo, bar };. Instead, write const obj = { foo: foo, bar: bar };." At draft-jobs.component.ts:926, the newly added createSpreadsheetRows method uses durationMinutes, (property shorthand) inside the returned object literal instead of durationMinutes: durationMinutes,.

Suggested change

	durationMinutes,
	durationMinutes: durationMinutes,

---

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

[devin-ai-integration]

🚩 Hard-coded color values in new theme file

Lines 16-18 and 22-23 use hard-coded hex/named colors (#4fa3ff, #007fff, lightGreen, darkGreen, #dbdb00, #ffcc00) for status and warning indicators, whereas other status colors in the same file properly use mat.get-theme-color(). The AGENTS.md rule says "Avoid making up and using hard-coded or new color values. Where feasible, use color values from Material Design." Many of these colors have approximate Material Design equivalents (e.g., the primary palette for 'active' blue, the tertiary/success palette for 'completed' green). The inconsistency within the file itself is notable — faulted correctly uses mat.get-theme-color($theme, error, ...) while completed uses lightGreen/darkGreen.

---

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

[devin-ai-integration]

🚩 sfAcknowledgedCompletion timeline field will always be null for builds from GetBuildsSinceAsync

The FindEventTime method at src/SIL.XForge.Scripture/Services/MachineApiService.cs:1540-1575 correlates events by matching either e.Result == servalBuildId or the draftGenerationRequestId tag. For BuildCompletedAsync events, neither match path works:

1. e.Result is null because BuildCompletedAsync returns Task (void) and captureReturnValue is not set (see IMachineApiService.cs:29).
2. GetRequestIdFromEvent(e) returns null because BuildCompletedAsync events don't carry the DraftGenerationRequestIdKey tag.

As a result, sfAcknowledgedCompletion in BuildReportTimeline will always be null. The frontend handles this gracefully (showing "-"), but it means the "SF acknowledges Serval completion" row in the Timing card is always empty, and percentTimeOnSF statistics will exclude most builds. The buildId is available in the event's Payload but FindEventTime only checks Result and tags.

---

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

[devin-ai-integration]

🚩 requesterDisplayName creates a new Observable per call in the template without shareReplay

In serval-builds.component.ts:367-386, requesterDisplayName() is called from the template for each expanded row. It caches the Observable by user ID, but since the template calls it via requesterDisplayName(row.report.requesterSFUserId) | async, Angular's async pipe will subscribe each time the view re-renders. The from(this.userService.getProfile(...)) creates a cold Observable that replays the HTTP call on each subscription. Since the cache stores the Observable (not the result), re-subscriptions on the same Observable would re-trigger the promise. Consider adding .pipe(shareReplay(1)) to the cached Observable to avoid redundant profile lookups when the view re-renders.

---

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

[devin-ai-integration]

🚩 interpretTypes throws on unknown status, failing the entire batch

In serval-build-report.ts:92-95, interpretTypes throws an Error for any status string not in the DraftGenerationBuildState set. This function is applied via reports.map(report => interpretTypes(report)) at draft-generation.service.ts:162. If Serval introduces a new build state in the future, a single report with the new status would cause the entire map() to throw, which the catchError at draft-generation.service.ts:120-128 would catch — resulting in ALL build reports being lost and a generic error shown to the admin. Consider either skipping unknown-status reports (with a warning) or falling back to a raw string status, so the other reports remain usable.

---

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

[devin-ai-integration]

🚩 GetBuildsSinceAsync does not filter by pre-translate engine type

The new GetBuildsSinceAsync in MachineApiService.cs:1127 calls translationBuildsClient.GetAllBuildsCreatedAfterAsync which retrieves all translation builds across all engines. Unlike GetBuildsAsync (which explicitly gets the pre-translation engine ID and calls GetAllBuildsAsync on that specific engine), this endpoint returns builds for both SMT and NMT engines. The BuildEngineToProjectMapAsync method matches engines via both PreTranslationEngineId and TranslationEngineId, so SMT builds will also be included in the results. This may be intentional (showing all Serval builds for admin visibility), but it's worth noting that the existing Draft Jobs tab only shows pre-translation builds.

---

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