[RaymondLuong3]

The serval administration page on specific projects have a button that opens to the serval builds page. The serval builds used to open with the default date range of 2 weeks which obscures the results. This PR updates the Serval builds page to accept a projectId query parameter that skips the date range filter so all results are displayed.

The endpoint GetBuildsSince has been modified so that the dateTime parameter is optional. Note that this breaks the endpoint for older clients. Since this is a serval administration endpoint, that shouldn't be a big issue.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 76.47059% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (b10583e) to head (7307e1b).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...orge.Scripture/Controllers/MachineApiController.cs	0.00%	8 Missing ⚠️
...p/serval-administration/serval-builds.component.ts	82.60%	3 Missing and 1 partial ⚠️
...SIL.XForge.Scripture/Services/MachineApiService.cs	85.71%	3 Missing and 1 partial ⚠️
...uest-detail/onboarding-request-detail.component.ts	0.00%	2 Missing ⚠️
...serval-administration/serval-projects.component.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4176   +/-   ##
=======================================
  Coverage   81.23%   81.24%           
=======================================
  Files         671      671           
  Lines       42990    43040   +50     
  Branches     7121     7139   +18     
=======================================
+ Hits        34923    34967   +44     
- Misses       6902     6923   +21     
+ Partials     1165     1150   -15     

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

[github-actions]

✅ No screenshot differences — all stories are identical.

View the diff page at: https://pr-4176--sf-screenshot-diffs.netlify.app

[pmachapman]

@pmachapman reviewed 11 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on RaymondLuong3).

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

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

    combineLatest([
      this.onlineStatusService.onlineStatus$,
      this.dateRange$,

dateRange$ initially contains undefined. This causes an initial request with no date filtering if you visit the tab without a project selected. Then a second request with date filtering is sent:

The result is the slower request ends up having its data rendered. On live this means default behavior will be rendering all builds since 2025-12-01, which will likely cause the tab to hang and/or crash.

The solution is to not request the builds until range != null || noDateRange

[RaymondLuong3]

@RaymondLuong3 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).

---

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

Previously, Nateowami wrote…

dateRange$ initially contains undefined. This causes an initial request with no date filtering if you visit the tab without a project selected. Then a second request with date filtering is sent:

The result is the slower request ends up having its data rendered. On live this means default behavior will be rendering all builds since 2025-12-01, which will likely cause the tab to hang and/or crash.

The solution is to not request the builds until range != null || noDateRange

You are right. The dateRange$ observable fires immediately with the undefined value which will cause the component to initially load the builds for all time. I thought about making this a standard Subject, but I experienced race conditions previously with the useSpecifiedDateRange$ observable. So I think it is safer to make this just check whether undefined is valid for the date range.

[Nateowami]

@Nateowami made 3 comments.
Reviewable status: 10 of 11 files reviewed, 4 unresolved discussions (waiting on pmachapman and RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 580 at r1 (raw file):
This is loading all builds. The Jira issue calls for only loading builds for the active project:

instead send a query to the server that requests all builds for the given project ID

This PR also doesn't add the toolbar control the issue calls for.

---

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

        }

        this.useUnspecifiedDateRange$.next(!!saParams.noDateRange);

saParams.noDateRange is a string or undefined. !!'false' === true. So this line will always set it to true if the value is in the URL at all. You could do something like String(saParams.noDateRange) === 'true'. And also update the type to admit it's a string not a boolean.

Or it might be simpler to just drop this and have a projectID param, and when that's set, default the date range to all dates (this is what the Jira issue actually calls for)

---

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

      }

      const rangeStart: Date = range != null ? range.start : new Date('2025-12-01');

Why this cutoff? (Even if it's completely right, it needs a comment explaining what the date is). Or a variable name that makes clear what the value corresponds to (e.g. const dateSuchAndSuchWasAddedToDataModel = new Date(...)

[RaymondLuong3]

The terniary operator would work here, but this is fine too.

[RaymondLuong3]

@RaymondLuong3 made 5 comments and resolved 1 discussion.
Reviewable status: 5 of 19 files reviewed, 5 unresolved discussions (waiting on Nateowami and pmachapman).

---

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

Previously, Nateowami wrote…

saParams.noDateRange is a string or undefined. !!'false' === true. So this line will always set it to true if the value is in the URL at all. You could do something like String(saParams.noDateRange) === 'true'. And also update the type to admit it's a string not a boolean.

Or it might be simpler to just drop this and have a projectID param, and when that's set, default the date range to all dates (this is what the Jira issue actually calls for)

Thanks for catching that bug. I have updated the implementation so that if the projectId param is set, then the query will default to use unspecified date range.

---

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

Previously, Nateowami wrote…

Why this cutoff? (Even if it's completely right, it needs a comment explaining what the date is). Or a variable name that makes clear what the value corresponds to (e.g. const dateSuchAndSuchWasAddedToDataModel = new Date(...)

It looks like Serval Client 1.12 (release Nov 20) was the first to include the GetBuildSince endpoint. However, the date requirement is optional. I updated the since parameter in getBuildsSince to be optional.

---

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

Previously, Nateowami wrote…

This is loading all builds. The Jira issue calls for only loading builds for the active project:

instead send a query to the server that requests all builds for the given project ID

This PR also doesn't add the toolbar control the issue calls for.

I copied the notice from the draft jobs tab.

---

src/SIL.XForge.Scripture/Controllers/MachineApiController.cs line 273 at r4 (raw file):

    [HttpGet(MachineApi.GetBuildsSince)]
    public async Task<ActionResult<IReadOnlyList<ServalBuildReportDto>>> GetBuildsSinceAsync(
        [FromQuery] string? dateTime,

This change will break older clients and they will not be able to invoke this endpoint. However, since this is a serval admin endpoint, I think this is acceptable.

Code quote:

        [FromQuery] string? dateTime,

[RaymondLuong3]

The terniary operator would work here, but this is fine too.

[Nateowami]

I'm not seeing it show the project name in the notice when there are no builds:

Whereas on the draft jobs tab it does show the name:

I think the problem is in how the project name is loaded. And the project I'm looking at has no builds, which appears to be how the component is loading the project's name. You could look at the draft jobs component to see how it loads the project name, but I think it just loads the project profile. Here's what it does:

      // Fetch project name if filtering by project
      let displayName = projectFilterId ?? '';
      if (projectFilterId != null) {
        try {
          const projectDoc = await this.servalAdministrationService.get(projectFilterId);
          displayName = projectDoc?.data != null ? projectLabel(projectDoc.data) : projectFilterId;
        } catch (error) {
          // We can filter for a now-deleted project, so an error here is not unexpected and fully supported
          console.error('Error fetching project name:', error);
        }
      }

@Nateowami made 4 comments and resolved 3 discussions.
Reviewable status: 5 of 19 files reviewed, 5 unresolved discussions (waiting on pmachapman and RaymondLuong3).

---

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

      <div class="filter-notice-content">
        <span>
          Showing draft jobs for project <strong>{{ filteredProjectName || currentProjectFilter }}</strong>

For consistency with the rest of the component this should probably say showing Serval builds rather than draft jobs.

---

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

        }

        IList<TranslationBuild> builds = await translationEnginesClient.GetAllBuildsAsync(

Do you think maybe this should be wrapped in a try catch so if the engine is missing in Serval it doesn't cause an error and instead lists zero builds?

IList<TranslationBuild> builds;  
try
  {
      builds = await translationEnginesClient.GetAllBuildsAsync(translationEngineId, cancellationToken);
  } 
  catch (ServalApiException e) when (e.StatusCode == StatusCodes.Status404NotFound)
  {
      return [];
  }

Any time SF deletes an engine we also clear our record of it, so it should hit the catch (DataNotFoundException) above, but it seems like if Serval doesn't have the engine we should still handle it correctly. Otherwise it causes a 500 error.

---

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

                nameof(BuildCompletedAsync),
            ],
            fromDate: since?.UtcDateTime ?? null

?? null is redundant

[RaymondLuong3]

You are correct. I've updated serval builds component to follow the pattern in the draft jobs component.

@RaymondLuong3 made 3 comments and resolved 3 discussions.
Reviewable status: 5 of 19 files reviewed, 2 unresolved discussions (waiting on Nateowami and pmachapman).

---

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

Previously, Nateowami wrote…

Do you think maybe this should be wrapped in a try catch so if the engine is missing in Serval it doesn't cause an error and instead lists zero builds?

IList<TranslationBuild> builds;  
try
  {
      builds = await translationEnginesClient.GetAllBuildsAsync(translationEngineId, cancellationToken);
  } 
  catch (ServalApiException e) when (e.StatusCode == StatusCodes.Status404NotFound)
  {
      return [];
  }

Any time SF deletes an engine we also clear our record of it, so it should hit the catch (DataNotFoundException) above, but it seems like if Serval doesn't have the engine we should still handle it correctly. Otherwise it causes a 500 error.

I imagine in some rare case where an error prevented a translation engine Id from being deleted from a project could lead to the project having that record but the translation engine deleted in serval. I have wrapped this in a try catch.

---

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

Previously, Nateowami wrote…

?? null is redundant

Removed

[RaymondLuong3]

@Nateowami let me know if there is something more you would like to see

[Nateowami]

@RaymondLuong3 Sorry I've taken so long to get back to this. I've been thinking about it every day since your latest comment but didn't get to it.

I'm fine with merging it as-is, however, there is at least one bug; if you clear the filter and then hit the back button, it says it's filtering, but it's not. It's not that important, but if the test team finds it and then you decide to fix it, it will be another back-and-forth. However, I think part of the reason for that bug is the way startWithEmptyRange works. I think it would make more sense for the component to just have a value set and not have a concept of a default.

I think my recommendation will be merge without fixing the bug, and let a fix for that be a followup.

I'm going to set the issue as ready to test.

@Nateowami made 2 comments and resolved 2 discussions.
Reviewable status: 5 of 19 files reviewed, all discussions resolved (waiting on pmachapman).

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

I imagine in some rare case where an error prevented a translation engine Id from being deleted from a project could lead to the project having that record but the translation engine deleted in serval. I have wrapped this in a try catch.

Looks good. Thanks.

[pmachapman]

@pmachapman reviewed 14 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on RaymondLuong3).