Skip to content

SF-3757 Simplify draft sources configuration page#3763

Open
Nateowami wants to merge 1 commit intomasterfrom
feature/SF-3757-simplify-draft-config
Open

SF-3757 Simplify draft sources configuration page#3763
Nateowami wants to merge 1 commit intomasterfrom
feature/SF-3757-simplify-draft-config

Conversation

@Nateowami
Copy link
Copy Markdown
Collaborator

@Nateowami Nateowami commented Mar 27, 2026

This PR creates a new, simplified page for configuring draft sources.

localhost_5000_projects_69c68548c30722fa1ac278c3_draft-generation

I initially made changes to the draft sources component, moving the content from the left, while keeping the layout of the overview on the right. See diagram below:

Screenshot from 2026-03-27 13-39-43

During a recent meeting we discussed the best way to get user feedback before rolling this out. It was decided to create a new "Experimental features" system that allows users to turn it on, so that it can be tested with real users and real projects before we roll it out.

I created an experimental features service and dialog that merely exposes certain feature flags through a different UI to allow users to turn them on or off. When no experimental features are available, it's not available in the menu. Some features (such as the new configure sources page) are only available to users that are an admin on at least one project.

Mostly I deleted a lot of code, especially in the template and styles, plus a few things in the component TS file that were no longer used by the template. Unfortunately, copying the component makes it a lot harder for a reviewer to tell what was changed.


This change is Reviewable


Open with Devin

@Nateowami Nateowami added will require testing PR should not be merged until testers confirm testing is complete e2e Run e2e tests for this pull request labels Mar 27, 2026
@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 151ad27 to e0e3d80 Compare March 27, 2026 21:07
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.28%. Comparing base (50ffeaf) to head (62d6b92).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ientApp/src/xforge-common/user-projects.service.ts 0.00% 7 Missing ⚠️
...ate/draft-generation/draft-generation.component.ts 66.66% 1 Missing ⚠️
.../ClientApp/src/xforge-common/external-url-class.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3763      +/-   ##
==========================================
+ Coverage   81.27%   81.28%   +0.01%     
==========================================
  Files         622      622              
  Lines       39322    39317       -5     
  Branches     6391     6416      +25     
==========================================
+ Hits        31958    31959       +1     
+ Misses       6379     6363      -16     
- Partials      985      995      +10     

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

@Nateowami Nateowami marked this pull request as ready for review March 30, 2026 15:00
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
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.

@Nateowami made 12 comments.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions.


scripts/db_tools/parse-version.ts line 36 at r1 (raw file):

    'Use In-Process Machine for Suggestions',
    'Use Serval for Suggestions',
    'Allow Echo for Pre-Translation Drafting',

The changes here are to bring the strings in line with how they're written in the feature flag service.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html line 100 at r1 (raw file):

          ></app-project-select>
        }
        <p class="no-bottom-margin">

The bottom margin was only hidden because the training-data-multi-select component created padding it shouldn't have created. Once that was fixed, this needed to be removed.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 5 at r1 (raw file):

  <app-notice class="experimental-notice" type="primary" mode="fill-dark" icon="science">
    This is an experimental simplified page for configuring sources. Please

This text is intentionally not localized as it will only exist while this is available as an experimental feature.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 10 at r1 (raw file):

  <mat-expansion-panel class="introduction">
    <mat-expansion-panel-header>How to configure draft sources</mat-expansion-panel-header>

This intro area is intentionally not localized as we are nowhere close to settled on wording.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/supported-back-translation-languages-dialog/supported-back-translation-languages-dialog.component.ts line 11 at r1 (raw file):

@Component({
  selector: 'app-supported-back-translation-languages-dialog',
  imports: [MatIcon, MatIconButton, MatDialogTitle, MatDialogContent, MatDialogClose, TranslocoModule],

The close button for the dialog did not have the correct appearance because this module wasn't included.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.html line 2 at r1 (raw file):

<ng-container *transloco="let t; read: 'training_data_multi_select'">
  @if (availableTrainingData.length > 0) {

The mat-list had padding or margin on top and bottom, which created spacing even when the list is empty. This creaetd challenges with proper spacing in the parent component.


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):

    "unknown_language_code": "unknown"
  },
  "new_configure_sources": {

I wasn't completely certain how to handle this. We're mostly keeping the same wording as the existing component, but two strings have been edited, so I put them in their own section.

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from e0e3d80 to acd14da Compare March 30, 2026 17:11
devin-ai-integration[bot]

This comment was marked as resolved.

@RaymondLuong3 RaymondLuong3 self-assigned this Apr 2, 2026
@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from acd14da to f344e55 Compare April 2, 2026 18:18
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to updated scripts/parse-version.ts with you new feature flag. Otherwise, the experimental feature service might be handy as we iterate on other designs :)

@RaymondLuong3 reviewed 25 files and all commit messages, made 7 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Nateowami).


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):

Previously, Nateowami wrote…

I wasn't completely certain how to handle this. We're mostly keeping the same wording as the existing component, but two strings have been edited, so I put them in their own section.

Yea, it is possible putting it into the original draft_sources section would have been fine. This section should be merged with the original once the new draft sources component is merged.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 38 at r3 (raw file):

      The model will create drafts by translating books from the 'Drafting Source Project'. It is rare that the model
      will give useful results if the Drafting Source Project isn't included in the Source projects for training the
      model.

This is strange. Instead of telling me why it is bad to not use the source project, it would be better to tell me the advantages of using the source project for drafting.

Code quote:

      The model will create drafts by translating books from the 'Drafting Source Project'. It is rare that the model
      will give useful results if the Drafting Source Project isn't included in the Source projects for training the
      model.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 43 at r3 (raw file):

    <p>
      You will be able to look at the drafts before you need to decide how to use them. Scripture Forge will not
      automatically add the generated drafts to any project. They will only be available in Scripture Forge until you

Automatically is not needed.

Code quote:

automatically add the generated drafts to any project

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.scss line 35 at r3 (raw file):

    margin-bottom: 0em;
  }
  h3 + p {

Nice, I haven't seen this syntax before

Code quote:

  h3 + p {

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):

import { TrainingData } from 'realtime-server/lib/esm/scriptureforge/models/training-data';
import { filter, merge, Subscription } from 'rxjs';
import { NoticeComponent } from 'src/app/shared/notice/notice.component';

Nit: this could be a relative path

Code quote:

import { NoticeComponent } from 'src/app/shared/notice/notice.component';

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.scss line 28 at r3 (raw file):

  ::ng-deep .mdc-label {
    display: block;

I don't see this is doing anything.

Code quote:

    display: block;

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from f344e55 to fac7eca Compare April 7, 2026 17:31
@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from fac7eca to 5d563b0 Compare April 7, 2026 17:43
Copy link
Copy Markdown
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.

parse-version has already been updated.

@Nateowami made 6 comments and resolved 2 discussions.
Reviewable status: 23 of 25 files reviewed, 3 unresolved discussions (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 38 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This is strange. Instead of telling me why it is bad to not use the source project, it would be better to tell me the advantages of using the source project for drafting.

This wording was an initial draft. I've just removed it


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 43 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Automatically is not needed.

Ditto.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.scss line 35 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nice, I haven't seen this syntax before

It means a paragraph that comes right after an h3


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: this could be a relative path

...why?


src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.scss line 28 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I don't see this is doing anything.

Done.

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 5 potential issues.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +380 to +381
removedFiles.forEach(f => this.trainingDataService.deleteTrainingDataAsync(f));
addedFiles.forEach(f => this.trainingDataService.createTrainingDataAsync(f));
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Unawaited async promises in save() cause silent failures for training data operations

In save(), deleteTrainingDataAsync and createTrainingDataAsync return Promises that are called inside forEach without being awaited. If any of these operations fail, the error becomes an unhandled promise rejection while the settings update on line 390-393 proceeds regardless, potentially leaving training data in an inconsistent state with the saved project settings. This is the same pattern as the existing draft-sources.component.ts:399-400, but it's newly introduced code in this PR.

Open in Devin Review

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fixing any bugs in the original

}

@Injectable({ providedIn: 'root' })
export class ExperimentalFeaturesService {
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing class comment on ExperimentalFeaturesService (AGENTS.md violation)

AGENTS.md states: "All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious. Please do not fail to add a comment to any classes or interfaces that are created." The new ExperimentalFeaturesService class has no JSDoc comment describing its purpose.

Open in Devin Review

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

FormsModule
]
})
export class ExperimentalFeaturesDialogComponent {
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing class comment on ExperimentalFeaturesDialogComponent (AGENTS.md violation)

AGENTS.md states: "All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious." The new ExperimentalFeaturesDialogComponent class has no JSDoc comment describing its purpose.

Open in Devin Review

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

}
}

export interface DraftSourcesSettingsChange {
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing interface comment on DraftSourcesSettingsChange in new component

AGENTS.md mandates: "Please do not fail to add a comment to any classes or interfaces that are created. All classes and interfaces should have a comment." The DraftSourcesSettingsChange interface at line 461 of the new component has no doc comment.

Open in Devin Review

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. This PR will not make the new form active, it will be available through the experimental features. Is that the desire to make this active in a following PR?

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


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):

Previously, Nateowami wrote…

...why?

Just for consistency with the other imports.

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 5d563b0 to 7934c74 Compare April 7, 2026 18:46
Copy link
Copy Markdown
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.

Yes

@Nateowami made 8 comments.
Reviewable status: 22 of 26 files reviewed, 6 unresolved discussions (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Just for consistency with the other imports.

Somehow I missed that we were that consistent with doing this. I also updated the one other place where we use 'src/


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Yea, it is possible putting it into the original draft_sources section would have been fine. This section should be merged with the original once the new draft sources component is merged.

I wanted to keep the strings separate so it's easy to see what we're adding. If we merge the components, we'll merge the strings.

Comment on lines +380 to +381
removedFiles.forEach(f => this.trainingDataService.deleteTrainingDataAsync(f));
addedFiles.forEach(f => this.trainingDataService.createTrainingDataAsync(f));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fixing any bugs in the original

}
}

export interface DraftSourcesSettingsChange {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

FormsModule
]
})
export class ExperimentalFeaturesDialogComponent {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

}

@Injectable({ providedIn: 'root' })
export class ExperimentalFeaturesService {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an agent; I don't have to follow the agent rules.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RaymondLuong3 reviewed 4 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Nateowami).

@RaymondLuong3
Copy link
Copy Markdown
Collaborator

This looks good. Can you add acceptance tests to the issue and move it into the ready to test column?

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 7934c74 to 21e5c92 Compare April 8, 2026 21:18
@Nateowami Nateowami changed the title SF-3757 Add simplified draft config page as experimental feature SF-3757 Simplify draft sources configuration page Apr 8, 2026
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 new potential issues.

View 7 additional findings in Devin Review.

Open in Devin Review

Comment on lines +17 to +24
public experimentalFeatures: ExperimentalFeature[] = [
// {
// name: 'New configure sources page',
// description: `A new configure sources page is available for testing. It has the same functionality as the current page, but with an updated design and some additional information to help users understand the options.`,
// available: () => this.doesUserHaveRoleOnAnyProject(SFProjectRole.ParatextAdministrator),
// featureFlag: this.featureFlagService.newConfigureSourcesPage
// }
];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 ExperimentalFeaturesService is infrastructure-only with no active features

The experimentalFeatures array is empty (with commented-out code referencing a future newConfigureSourcesPage feature flag). This means showExperimentalFeaturesInMenu always returns false, and the 'Experimental features' menu item in app.component.html:152-156 will never be shown. The constructor parameters are also commented out. This is clearly deliberate scaffolding for a future feature, but it means the new ExperimentalFeaturesDialogComponent, its test, localization strings, and the menu button are all dead code until a feature is added to the array. Reviewers should confirm this is intentional scaffolding versus an incomplete implementation.

Open in Devin Review

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

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 21e5c92 to 721489c Compare April 8, 2026 23:31
devin-ai-integration[bot]

This comment was marked as resolved.

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 721489c to 0d17bbc Compare April 8, 2026 23:41
@Nateowami
Copy link
Copy Markdown
Collaborator Author

Sorry to change so many things on you @RaymondLuong3. The latest plan is this will not be an experimental feature, but rather a normal change. I've also made some significant adjustments to the layout of the component so the inputs are aligned both vertically and horizontally.

@Nateowami Nateowami force-pushed the feature/SF-3757-simplify-draft-config branch from 0d17bbc to 62d6b92 Compare April 9, 2026 00:04
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

Open in Devin Review

Comment on lines +28 to +35
<button
mat-button
(click)="trainingSources.push(undefined); $event.preventDefault()"
class="add-another-project"
>
<mat-icon>add</mat-icon> {{ t("add_another_reference_project") }}
</button>
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Stepper removal means empty project-select slots can no longer be cleaned up

The old goToStep() method cleaned up undefined entries from trainingSources/draftingSources arrays when navigating between steps. With the stepper removed, there's no cleanup mechanism. If a user clicks "Add another reference project" but doesn't select anything, the empty slot persists with no way to remove it (the "Add another reference project" button disappears because allowAddingATrainingSource checks trainingSources.length < 2). This doesn't cause a functional bug — save() filters out undefined values with filter(notNull) — but it's a minor UX regression where users can't undo adding an empty slot.

Open in Devin Review

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run e2e tests for this pull request will require testing PR should not be merged until testers confirm testing is complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants