serval-admin: serval-builds: show Problems

[marksvc]

This patch adds a Problems card to the Serval build record. The card
displays an abbreviated list of various errors or warnings that were
associated with the build. A button opens a dialog with unabbreviated
information.

The ServalBuildReportDto class now carries a more descriptive Problems
field.

The ServalBuildExecutionData is modified to also include its Warnings.
This is redundant with a subset of ServalBuildReportDto.Problems but
seemed appropriate to add.

The GUI change is in Serval Administration > Serval Builds tab. Expand a build record to see cards.
Screenshot of Problems card with no problems:

Screenshot of Problems card with mock problems:

Screenshot of resulting dialog after clicking Show all:

Clipboard contents after clicking Copy all:

# Problems with Serval build ID 69fa6fffbf2b2e708b149a13

## SF errors

- Big local problem! Build is 
12341232134
1234423141234123423
123442312341423412241423234412323412341
12342341234123412341

## Serval errors

- Big problem here at Serval! 432114231234234112341234123441234124123412

## Serval warnings

- Minor problem here at Serval. 12341234123412341234
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.
- RUT 0:2 has bad versification in the verses and will not be formatted as verses.

---

This change is 

[Copilot]

Pull request overview

Adds a "Problems" card to the Serval build admin view that displays an abbreviated list of build errors/warnings, with a dialog for the full set. Reshapes the Problems field from string[] to a structured BuildReportProblem[] (source + severity + message) on the backend DTO, and exposes Serval execution-data warnings.

Changes:

• New BuildReportProblem model + BuildReportProblemSource/Severity enums; backend CollectProblems aggregates SF event exceptions, faulted state messages, and Serval execution-data warnings.
• New ServalBuildProblemsDialog (Angular standalone) with markdown copy-all support; new "Problems" card in the build details view with preview-and-truncate behavior.
• TS DTO updated with the new problem structure and runtime validation in interpretTypes.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/SIL.XForge.Scripture/Models/BuildReportProblem.cs	New problem model with source/severity enums.
src/SIL.XForge.Scripture/Models/ServalBuildReportDto.cs	Problems field changed from List<string> to List<BuildReportProblem>.
src/SIL.XForge.Scripture/Models/ServalBuildExecutionData.cs	Adds Warnings list.
src/SIL.XForge.Scripture/Services/MachineApiService.cs	Adds CollectProblems, populates problems including execution-data warnings; updates fallback "no Serval build" entries.
ClientApp/.../machine-api/build-dto.ts	Adds warnings to BuildExecutionData.
ClientApp/.../serval-administration/serval-build-report.ts	Adds BuildReportProblem interface, source/severity types, runtime validation.
ClientApp/.../serval-build-problems-dialog.component.{ts,html,scss,spec.ts}	New dialog component, template, styles, and unit tests.
ClientApp/.../serval-builds.component.{ts,html,scss,spec.ts}	New "Problems" card, badge tooltip, helper methods, and unit tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marksvc]

Done.

[marksvc]

Done.

[marksvc]

There are various things we could do here. I'm not too concerned about how much information the warning badge does or doesn't convey when we already have a big red Faulted indication.

[marksvc]

Hmm. That's true. Since it's all nice and tested right now, and since using problemSections() would change the governing order to be from the problemSections() output order rather than the template-defined order, I'm going to leave it like it is for now, unless further discussion.

[codecov]

Codecov Report

❌ Patch coverage is 44.55446% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.96%. Comparing base (021000d) to head (a539639).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs	26.15%	45 Missing and 3 partials ⚠️
...c/app/serval-administration/serval-build-report.ts	14.28%	6 Missing ⚠️
...stration/serval-build-problems-dialog.component.ts	90.00%	1 Missing ⚠️
...p/serval-administration/serval-builds.component.ts	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3880      +/-   ##
==========================================
- Coverage   81.05%   80.96%   -0.10%     
==========================================
  Files         630      632       +2     
  Lines       40690    40788      +98     
  Branches     6603     6614      +11     
==========================================
+ Hits        32983    33025      +42     
- Misses       6673     6725      +52     
- Partials     1034     1038       +4     

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

[RaymondLuong3]

Nice work! I think this will be helpful to serval admins. I do wonder if we should separate errors from warnings since fundamentally those are two different sets of problems.

@RaymondLuong3 reviewed 14 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-problems-dialog.component.html line 29 at r2 (raw file):

    <mat-icon>content_copy</mat-icon>
    Copy all
  </button>

What do you suppose that the copy function will be useful for? I am guessing if there are many issues, then you can copy it and paste it in a file to do a search?

Code quote:

  <button mat-button (click)="copyAllToClipboard()">
    <mat-icon>content_copy</mat-icon>
    Copy all
  </button>

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.ts line 57 at r2 (raw file):

  }

  /** Copies all problems to the clipboard. */

Some of these method comments aren't necessary. This one for sure.

Code quote:

  /** Copies all problems to the clipboard. */

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.spec.ts line 43 at r2 (raw file):

    expect(result).toContain('- SF failure');
    expect(result).toContain('## Serval warnings');
    expect(result).toContain('- Low confidence');

You can assign these strings to variables so that they can be reused between tests in this suite.

Code quote:

    expect(result).toContain('## SF errors');
    expect(result).toContain('- SF failure');
    expect(result).toContain('## Serval warnings');
    expect(result).toContain('- Low confidence');

---

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

                    @let sfWarnings = problems(row, "local", "warning");
                    @let servalErrors = problems(row, "serval", "error");
                    @let servalWarnings = problems(row, "serval", "warning");

I would probably assign these in the typescript rather than in the html. And I would also use the types that you defined rather than the string values.

Code quote:

                    @let sfErrors = problems(row, "local", "error");
                    @let sfWarnings = problems(row, "local", "warning");
                    @let servalErrors = problems(row, "serval", "error");
                    @let servalWarnings = problems(row, "serval", "warning");

---

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

    row: ServalBuildRow,
    source: BuildReportProblem['source'],
    severity: BuildReportProblem['severity']

I don't understand this syntax. Is there an easier way to write this?

Code quote:

    source: BuildReportProblem['source'],
    severity: BuildReportProblem['severity']

---

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

  }

  protected showAllProblems(row: ServalBuildRow): void {

I don't think there is any advantage in making these methods protected since they do not modify data. Make them public, and then the tests can access them directly.

Code quote:

  protected showAllProblems(row: ServalBuildRow): void {

---

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

      const sections: { heading: string; problems: BuildReportProblem[] }[] = env.component['problemSections'](row);

      expect(sections[0].heading).toBe('SF errors');

Capture these constants in variables that can be reused through this spec

---

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

    /// and any SF event exceptions.
    /// </summary>
    private static List<BuildReportProblem> CollectProblems(

Nit: I wonder why the word collect doesn't sound right here. Maybe extract?

Code quote:

    private static List<BuildReportProblem> CollectProblems(

[marksvc]

Thank you. I hope so. Hmm, that's useful to consider. There are in fact different kinds of things going wrong. For example, server communication and USFM syntax issues are different things going wrong. I'm not sure that we have enough categorization or definition from Serval regarding what some of its messaging or warnings will be about. But we could start to split up some of the information onto different areas of the Serval builds page. Certainly for showing error or warning messages to users we may find that some of the error and warning areas are more suitable than others.

@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-problems-dialog.component.html line 29 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

What do you suppose that the copy function will be useful for? I am guessing if there are many issues, then you can copy it and paste it in a file to do a search?

I find that when I engage on a ticket, or investigate a specific problem, I am copying SF project IDs, sync metric IDs, and error messages, and pasting them either into my notes or into the ticket or jira issue. I suspect that when Serval admins use this page, they might need to copy the Serval warnings to ask the Serval developers about it, or to provide some information to the user (before we get SF to show the warnings to users directly). Serval admins might also have reason to copy an SF error exception message and paste it into chat for the SF team to look at.
That's what I'm thinking.

The Copy all button is big and comprehensive. Perhaps after using it a bit it will be clear whether the "Copy all" button is helpful vs just the section-specific copy buttons.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.ts line 57 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Some of these method comments aren't necessary. This one for sure.

Okay. I thought the methods were a bit confusing. :)
I removed the comments.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.spec.ts line 43 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

You can assign these strings to variables so that they can be reused between tests in this suite.

Done. (Although, were you wanting the strings passed to createSection() to also be using the variables?)

---

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

I would probably assign these in the typescript rather than in the html.

Thank you. Can you explain a bit more what you are meaning? These problems are specific to a row. So we wouldn't have, say, a
ServalBuildsComponent.sfErrors: BuildReportProblem[] field to assign to, because we would need one for each row.

Now, we could have a field something like
ServalBuildsComponent.sfErrors: Map<ServalBuildRow, BuildReportProblem[]> = new Map() , which would let us store all the rows' SF Error section problems, keyed by row. So in the component we could write

this.rows.forEach((row: ServalBuildRow) => {
      this.sfErrors.set( row, this.problems(row,'local','error') );
}

and then in the template we wouldn't need to
@let sfErrors = problems(row, "local", "error"); and could replace usage of
sfErrors with
sfErrors.get(row).

I'm guessing that when you wrote your comment, you were thinking in the context of these being specific to the component, rather than being row-specific. But I might not have understood what you were meaning. Thank you for explaining some more.

And I would also use the types that you defined rather than the string values.

Right. Sorry, the type was a less clear when I posted this. The source is BuildReportProblemSource, which is a union of 'local' | 'serval', so here specifying "local" or "serval" is sort of like picking parts of an enum. Severity is similar ('warning' | 'error'). Sorry for the confusion.

Oh I just realized that you might have been remembering the C# enum, where Source and Severity are specified like BuildReportProblemsSource.Serval and BuildreportProblemSevirity.Warning. But it's currently a string union type here.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

I don't understand this syntax. Is there an easier way to write this?

Sorry. When I look at it now it definitely seems preferable to just name the type! I changed these two lines to the names of the types.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

I don't think there is any advantage in making these methods protected since they do not modify data. Make them public, and then the tests can access them directly.

I think a general design principle here is to make methods public that other classes can/should call, and make other methods private that are not part of the 'public API'. A wrench is thrown in when wanting to write tests for a method that is not part of the public API. In C# we can make a method internal to help tests see it. TypeScript lets us use the component['privateMethodFoo'] syntax. I've also heard it suggested that one just shouldn't ever test non-public methods.

I agree that it isn't that significant what the ServallBuidsComponent public API is. And so lots of currently non-public methods could be made to be public. The distinction between public and non-public is more important for us in non-components, like services.

I changed these methods to be public.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Capture these constants in variables that can be reused through this spec

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: I wonder why the word collect doesn't sound right here. Maybe extract?

I renamed it to ExtractProblems.

[RaymondLuong3]

Nice, just one more change to make

@RaymondLuong3 reviewed 6 files and all commit messages, made 4 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.spec.ts line 43 at r2 (raw file):

Previously, marksvc wrote…

Done. (Although, were you wanting the strings passed to createSection() to also be using the variables?)

Yes, that would make sense to me

---

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

Previously, marksvc wrote…

I would probably assign these in the typescript rather than in the html.

Thank you. Can you explain a bit more what you are meaning? These problems are specific to a row. So we wouldn't have, say, a
ServalBuildsComponent.sfErrors: BuildReportProblem[] field to assign to, because we would need one for each row.

Now, we could have a field something like
ServalBuildsComponent.sfErrors: Map<ServalBuildRow, BuildReportProblem[]> = new Map() , which would let us store all the rows' SF Error section problems, keyed by row. So in the component we could write

this.rows.forEach((row: ServalBuildRow) => {
      this.sfErrors.set( row, this.problems(row,'local','error') );
}

and then in the template we wouldn't need to
@let sfErrors = problems(row, "local", "error"); and could replace usage of
sfErrors with
sfErrors.get(row).

I'm guessing that when you wrote your comment, you were thinking in the context of these being specific to the component, rather than being row-specific. But I might not have understood what you were meaning. Thank you for explaining some more.

And I would also use the types that you defined rather than the string values.

Right. Sorry, the type was a less clear when I posted this. The source is BuildReportProblemSource, which is a union of 'local' | 'serval', so here specifying "local" or "serval" is sort of like picking parts of an enum. Severity is similar ('warning' | 'error'). Sorry for the confusion.

Oh I just realized that you might have been remembering the C# enum, where Source and Severity are specified like BuildReportProblemsSource.Serval and BuildreportProblemSevirity.Warning. But it's currently a string union type here.

Ah, yes you caught on to my misunderstanding. I see now that it is row specific, so you would need to have a map of rows to problems. I think what you got here makes enough sense now that I look at it.

---

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

Previously, marksvc wrote…

I think a general design principle here is to make methods public that other classes can/should call, and make other methods private that are not part of the 'public API'. A wrench is thrown in when wanting to write tests for a method that is not part of the public API. In C# we can make a method internal to help tests see it. TypeScript lets us use the component['privateMethodFoo'] syntax. I've also heard it suggested that one just shouldn't ever test non-public methods.

I agree that it isn't that significant what the ServallBuidsComponent public API is. And so lots of currently non-public methods could be made to be public. The distinction between public and non-public is more important for us in non-components, like services.

I changed these methods to be public.

Yes, I agree that it is a little awkward to make public methods when we do not intend for other classes to call them. Just pragmatically speaking making these public is a simpler way to allow adequate testing without the tests being brittle since they can directly reference the public methods.

[marksvc]

@marksvc made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-problems-dialog.component.spec.ts line 43 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Yes, that would make sense to me

How about this.

[RaymondLuong3]

@RaymondLuong3 reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on marksvc).