Draft jobs table: Show caution for older data

[marksvc]

---

Screenshot:

This change is 

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.31%. Comparing base (b4cc938) to head (bc2c0fb).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3703   +/-   ##
=======================================
  Coverage   81.31%   81.31%           
=======================================
  Files         620      620           
  Lines       39076    39079    +3     
  Branches     6375     6352   -23     
=======================================
+ Hits        31775    31778    +3     
- Misses       6316     6329   +13     
+ Partials      985      972   -13     

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

[Nateowami]

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

---

.github/copilot-instructions.md line 117 at r1 (raw file):

# Tests

- Place a line with "// SUT" before the line that causes the code being tested to be exercised.

Can we add conditions and/or caveats to this? A lot of tests are simple enough that this would just be noise. Right now we have 168 instances of ``// SUTin*.ts` files, and 2,745 instances of `it(`. I'm sure some could use it that lack it, but I'm not sure a 16-fold increase in usage is called for.

---

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

  /** Whether to show a caution notice about comparing older durations. */
  get shouldShowDurationComparisonCaution(): boolean {

As a nit about naming, I think should is mostly redundant. That said, I do appreciate erring on the side of calrity, and not choosing a name like isDurationComparisonCuationShown.

---

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

    // Day after the date when draftGenerationRequestId began to be used, which was in SFv5.46.1 at
    // 2025-12-18T17:48:57Z.
    const requestIdIntroductionDate: Date = new Date(2025, 12 - 1, 18 + 1);

This would be way simpler as new Date(2025-12-18) (or you could use an exact timestamp) since everyone is more familiar with ISO date formats than JavaScript date APIs. Additionally, it's interpreted as UTC, whereas constructing a date from numbers results in it being interpreted as local time, meaning the cutoff depends on the user's timezone.

(Also in tests)

---

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

    const rangeStartMs: number = this.currentDateRange.start.getTime();
    const cutoffMs: number = requestIdIntroductionDate.getTime();

For what it's worth, you can directly compare JavaScript datetimes (and if you couldn't, TypeScript would yell at you for trying to compare incompatible types).

[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 1 additional finding.

[marksvc]

@marksvc made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Nateowami).

---

.github/copilot-instructions.md line 117 at r1 (raw file):

Previously, Nateowami wrote…

Can we add conditions and/or caveats to this? A lot of tests are simple enough that this would just be noise. Right now we have 168 instances of ``// SUTin*.ts` files, and 2,745 instances of `it(`. I'm sure some could use it that lack it, but I'm not sure a 16-fold increase in usage is called for.

Okay. I find it helpful even for short tests. How about if a unit test is >20 lines long?

From a rough examination of frontend tests, it looks like about 80% of tests are <= 20 lines, and 20% are >20 lines.

---

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

Previously, Nateowami wrote…

This would be way simpler as new Date(2025-12-18) (or you could use an exact timestamp) since everyone is more familiar with ISO date formats than JavaScript date APIs. Additionally, it's interpreted as UTC, whereas constructing a date from numbers results in it being interpreted as local time, meaning the cutoff depends on the user's timezone.

(Also in tests)

Thank you for that!

---

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

Previously, Nateowami wrote…

For what it's worth, you can directly compare JavaScript datetimes (and if you couldn't, TypeScript would yell at you for trying to compare incompatible types).

Done.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

[devin-ai-integration]

🟡 Caution notice shown when date range is entirely before the cutoff, not just when spanning it

The shouldShowDurationComparisonCaution getter only checks this.currentDateRange.start < requestIdIntroductionDate, but the warning message says "Be careful about comparing Serval build outcomes and durations before 2025-12-18 with those after". This implies the warning should only appear when the selected date range spans across the cutoff date (i.e., contains data from both before and after).

Root Cause

With the current condition at draft-jobs.component.ts:220, if a user selects a date range entirely before the cutoff (e.g., 2025-11-01 to 2025-12-01), start < requestIdIntroductionDate is true, so the caution notice is displayed. However, there is no data "after" the cutoff in that range, so the warning about comparing old data with new data is misleading.

The condition should also verify that the end date is after the cutoff:

return this.currentDateRange.start < requestIdIntroductionDate && this.currentDateRange.end > requestIdIntroductionDate;

Impact: Users viewing data entirely from before the cutoff date will see a spurious warning about comparing data across a boundary that their selection doesn't cross.

Suggested change

	return this.currentDateRange.start < requestIdIntroductionDate;
	return this.currentDateRange.start < requestIdIntroductionDate && this.currentDateRange.end > requestIdIntroductionDate;

---

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

[marksvc]

(I want the notice to show even if the user selects a start and end date that are both before the cutoff time. This is because if we consider duration or other measurements of that set of events, and compare it with measurements from a set of events that all occurred after the cutoff, there could be differences that arise from changes to how events are grouped. And in discussion, it was thought useful to display a cautionary notice.

The usefulness of the caution message is not limited to single queries of data with a range that spans the cutoff time.)

[marksvc]

(I want the notice to show even if the user selects a start and end date that are both before the cutoff time. This is because if we consider duration or other measurements of that set of events, and compare it with measurements from a set of events that all occurred after the cutoff, there could be differences that arise from changes to how events are grouped. And in discussion, it was thought useful to display a cautionary notice.

The usefulness of the caution message is not limited to single queries of data with a range that spans the cutoff time.)

[Nateowami]

@Nateowami partially reviewed 2 files and resolved 2 discussions.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on marksvc).

[marksvc]

Hey @Nateowami , thank you far having a look here. I'm a bit confused about the review status. It says you requested changes. Can you highlight where you still want some adjustments when you get the chance? Thank you!

[marksvc]

Hey @Nateowami , please let me know when you get the chance. Thanks!

[Nateowami]

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

---

AGENTS.md line 118 at r4 (raw file):

# Tests

- If a unit test is more than 20 lines long, then place a line with "// SUT" before the line that causes the code being tested to be exercised.

I'm sorry it's taken me so long to get back to this.

I'm really not sure 20 lines is a meaningful metric, but we can try this and see how it goes.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

[devin-ai-integration]

🚩 Caution notice shows for date ranges entirely before the cutoff

The shouldShowDurationComparisonCaution getter at src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts:214-224 returns true whenever rangeStartMs < cutoffMs, regardless of where the range ends. This means a date range like Nov 1–Nov 30, 2025 (entirely before the cutoff) will still trigger the warning "Be careful about comparing... before 2025-12-18 with those after", even though there is no post-cutoff data to compare against.

This isn't a bug per se — the warning is still defensible since the user is viewing pre-cutoff data — but it could be slightly confusing. A more precise check would be rangeStartMs < cutoffMs && rangeEndMs >= cutoffMs to only warn when the range actually straddles the boundary. Whether this matters depends on the intended UX.

---

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