Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ <h1>
@if (!isDraftInProgress(draftJob) && !hasAnyCompletedBuild) {
<section class="action-button-strip">
@if (isSourcesConfigurationComplete) {
<button mat-flat-button color="primary" (click)="generateDraft()">
<button mat-flat-button color="primary" (click)="generateDraftClicked()">
<mat-icon>auto_awesome</mat-icon>
{{ t("generate_draft_button") }}
</button>
Expand Down Expand Up @@ -358,7 +358,7 @@ <h3>{{ t("draft_finishing_header") }}</h3>
@if (hasDraftBooksAvailable) {
<app-draft-download-button [build]="lastCompletedBuild" />
@if (featureFlags.usfmFormat.enabled) {
<button mat-button [routerLink]="['format']">
<button mat-button [routerLink]="draftOptionsService.formattingOptionsPath">
<mat-icon>settings</mat-icon>
{{ t("format_draft") }}
</button>
Expand Down Expand Up @@ -391,16 +391,14 @@ <h3>{{ t("draft_finishing_header") }}</h3>
hasAnyCompletedBuild &&
isSourcesConfigurationComplete
) {
@if (!formattingOptionsSupported || formattingOptionsSelected) {
<button
mat-flat-button
color="primary"
type="button"
(click)="generateDraft({ withConfirm: !featureFlags.newDraftHistory.enabled })"
>
<mat-icon>add</mat-icon> {{ t("generate_new_draft") }}
</button>
}
<button
mat-flat-button
color="primary"
type="button"
(click)="generateDraftClicked({ withConfirm: !featureFlags.newDraftHistory.enabled })"
>
<mat-icon>add</mat-icon> {{ t("generate_new_draft") }}
</button>
@if (hasConfigureSourcePermission) {
<button mat-button data-test-id="configure-button" [routerLink]="['sources']">
<mat-icon>settings</mat-icon> {{ t("configure_sources") }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ export class DraftGenerationComponent extends DataLoadingComponent implements On
private readonly activatedProject: ActivatedProjectService,
private readonly authService: AuthService,
private readonly draftGenerationService: DraftGenerationService,
private readonly draftOptionsService: DraftOptionsService,
private readonly draftSourcesService: DraftSourcesService,
private readonly nllbService: NllbLanguageService,
protected readonly i18n: I18nService,
Expand All @@ -178,20 +177,13 @@ export class DraftGenerationComponent extends DataLoadingComponent implements On
protected readonly noticeService: NoticeService,
protected readonly urlService: ExternalUrlService,
protected readonly featureFlags: FeatureFlagService,
protected readonly draftOptionsService: DraftOptionsService,
private readonly projectService: SFProjectService,
private destroyRef: DestroyRef
) {
super(noticeService);
}

get formattingOptionsSelected(): boolean {
return this.draftOptionsService.areFormattingOptionsSelected();
}

get formattingOptionsSupported(): boolean {
return this.draftOptionsService.areFormattingOptionsSupportedForBuild(this.lastCompletedBuild);
}

get hasAnyCompletedBuild(): boolean {
return this.lastCompletedBuild != null;
}
Expand Down Expand Up @@ -342,7 +334,30 @@ export class DraftGenerationComponent extends DataLoadingComponent implements On
});
}

async generateDraft({ withConfirm = false } = {}): Promise<void> {
get formattingOptionsRequired(): boolean {
return (
this.draftOptionsService.areFormattingOptionsSupportedForBuild(this.lastCompletedBuild) &&
!this.draftOptionsService.areFormattingOptionsSelected()
);
}

async generateDraftClicked({ withConfirm = false } = {}): Promise<void> {
if (this.formattingOptionsRequired) {
const dialogRef = this.dialogService.openGenericDialog({
title: this.i18n.translate('draft_generation.choose_formatting_options'),
message: this.i18n.translate('draft_generation.choose_formatting_options_before_new_draft'),
options: [
{
value: true,
label: this.i18n.translate('draft_generation.formatting_options'),
highlight: true,
icon: 'build'
}
]
Comment on lines +349 to +356
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Dialog has no cancel/dismiss button, only an action button

The formatting options dialog in generateDraftClicked (draft-generation.component.ts:346-357) is created with only a single option (value: true, label "Formatting options"). Unlike every other dialog created via openGenericDialog in this codebase (e.g., the confirm dialogs at draft-generation.component.ts:362-373 and draft-generation.component.ts:385-396), this dialog provides no explicit cancel/close button. While the user can dismiss the dialog by pressing Escape or clicking the backdrop, this is not discoverable and inconsistent with the established UX pattern of always providing a cancel/close option in generic dialogs.

Suggested change
options: [
{
value: true,
label: this.i18n.translate('draft_generation.formatting_options'),
highlight: true,
icon: 'build'
}
]
options: [
{
value: false,
label: this.i18n.translate('dialog.cancel')
},
{
value: true,
label: this.i18n.translate('draft_generation.formatting_options'),
highlight: true,
icon: 'build'
}
]
Open in Devin Review

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

});
if (await dialogRef.result) await this.router.navigate(this.draftOptionsService.formattingOptionsPath);
return;
}
if (withConfirm) {
const isConfirmed: boolean | undefined = await this.dialogService.openGenericDialog({
title: this.i18n.translate('draft_generation.dialog_confirm_draft_regeneration_title'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
{{ t("select_formatting_options") }}
</p>
<div class="formatting-options-container">
<button matButton="filled" class="format-usfm" [routerLink]="['format']">
<button matButton="filled" class="format-usfm" [routerLink]="draftOptionsService.formattingOptionsPath">
<mat-icon>build</mat-icon>
<span class="hide-lt-lg">{{ t("formatting_options") }}</span>
<span class="hide-gt-lg">{{ t("formatting") }}</span>
Expand Down Expand Up @@ -56,7 +56,7 @@
</button>
<app-draft-download-button matButton="outlined" [build]="entry" />
@if (formattingOptionsSupported && isLatestBuild) {
<button matButton class="format-usfm" [routerLink]="['format']">
<button matButton class="format-usfm" [routerLink]="draftOptionsService.formattingOptionsPath">
<mat-icon>build</mat-icon> {{ t("formatting_options") }}
</button>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ export class DraftHistoryEntryComponent {
private readonly trainingDataService: TrainingDataService,
private readonly activatedProjectService: ActivatedProjectService,
readonly featureFlags: FeatureFlagService,
private readonly draftOptionsService: DraftOptionsService,
protected readonly draftOptionsService: DraftOptionsService,
private readonly permissionsService: PermissionsService,
private readonly destroyRef: DestroyRef,
private readonly dialog: MatDialog
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,8 @@ export class DraftOptionsService {
? new Date(entry.additionalInfo.dateFinished) > FORMATTING_OPTIONS_SUPPORTED_DATE
: false;
}

get formattingOptionsPath(): string[] {
return ['/projects', this.activatedProjectService.projectId!, 'draft-generation', 'format'];
}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment on lines +40 to +42
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Non-null assertion on projectId in formattingOptionsPath can produce invalid route

activatedProjectService.projectId is typed string | undefined, but the getter uses a non-null assertion (!) without any guard. If projectId is ever undefined (e.g., during initialization or a race condition), the resulting path becomes ['/projects', undefined, 'draft-generation', 'format'], which Angular coerces to the string "undefined", navigating the user to /projects/undefined/draft-generation/format — a non-existent route. This getter is called from three template bindings (draft-generation.component.html:361, draft-history-entry.component.html:29, draft-history-entry.component.html:59) and from draft-generation.component.ts:358 via this.router.navigate(...). Per the repository's AGENTS.md: "It is better to explicitly check for and handle problems, or prevent problems from happening, than to assume problems will not happen" and "Corner-cases happen. They should be handled in code."

Suggested change
get formattingOptionsPath(): string[] {
return ['/projects', this.activatedProjectService.projectId!, 'draft-generation', 'format'];
}
get formattingOptionsPath(): string[] {
const projectId: string = this.activatedProjectService.projectId ?? '';
return ['/projects', projectId, 'draft-generation', 'format'];
}
Open in Devin Review

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@
"successfully_applied_all_chapters": "Successfully applied all chapters to {{ bookName }}"
},
"draft_generation": {
"choose_formatting_options": "Choose formatting options to continue",
"choose_formatting_options_before_new_draft": "Before you can create a new draft, you need to choose formatting options.",
"formatting_options": "Formatting options",
"back_translation_requirement": "Back translation drafts can only be generated into the roughly 200 [link:supportedLanguagesUrl]supported languages[/link].",
"cancel_generation_button": "Cancel",
"configure_sources": "Configure sources",
Expand Down Expand Up @@ -299,7 +302,7 @@
"options_for_paragraph_breaks_and_quotation_marks": "Options for paragraph breaks and quotation marks to make editing easier",
"requested_at": "Requested {{ requestedAtTime }}.",
"requested_by": "Requested by {{ requestedByUserName }} at {{ requestedAtTime }}.",
"select_formatting_options": "The draft has been created, and there are new formatting options to select. You will only need to do this once, but you can change them whenever you wish.",
"select_formatting_options": "The draft has been created. Choose formatting options to continue.",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 Localization key 'select_formatting_options' changed text significantly without new key

The string for draft_history_entry.select_formatting_options was changed from a detailed explanation ("The draft has been created, and there are new formatting options to select. You will only need to do this once, but you can change them whenever you wish.") to a shorter message ("The draft has been created. Choose formatting options to continue."). Per REVIEW.md: "significant changes to a string should use a new key, otherwise existing translations will continue to show the old string." Since the meaning and length changed substantially, existing translations in other languages will show the old longer text until manually updated, which may be confusing.

Open in Devin Review

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

"show_model_training_configuration": "Show model training configuration",
"training_books": "Training books",
"training_data_files_used": "The following data files were used to train the model.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,18 @@ <h2 mat-dialog-title>{{ title }}</h2>
@for (option of options; track option.label) {
@if (option.highlight) {
<button mat-flat-button color="primary" [mat-dialog-close]="option.value">
@if (option.icon != null) {
<mat-icon>{{ option.icon }}</mat-icon>
}
{{ option.label | async }}
</button>
} @else {
<button mat-button [mat-dialog-close]="option.value">{{ option.label | async }}</button>
<button mat-button [mat-dialog-close]="option.value">
@if (option.icon != null) {
<mat-icon>{{ option.icon }}</mat-icon>
}
{{ option.label | async }}
</button>
}
}
</div>
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import { CdkScrollable } from '@angular/cdk/scrolling';
import { AsyncPipe } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { MatButton } from '@angular/material/button';
import {
MAT_DIALOG_DATA,
MatDialogRef,
MatDialogTitle,
MatDialogContent,
MatDialogActions,
MatDialogClose
MatDialogClose,
MatDialogContent,
MatDialogRef,
MatDialogTitle
} from '@angular/material/dialog';
import { MatIcon } from '@angular/material/icon';
import { Observable } from 'rxjs';
import { CdkScrollable } from '@angular/cdk/scrolling';
import { MatButton } from '@angular/material/button';
import { AsyncPipe } from '@angular/common';

export interface GenericDialogOptions<T> {
title?: Observable<string>;
Expand All @@ -19,6 +20,7 @@ export interface GenericDialogOptions<T> {
label: Observable<string>;
value: T;
highlight?: boolean;
icon?: string;
}[];
}

Expand All @@ -33,7 +35,16 @@ export interface GenericDialogRef<T> {
@Component({
selector: 'app-generic-dialog',
templateUrl: './generic-dialog.component.html',
imports: [MatDialogTitle, CdkScrollable, MatDialogContent, MatDialogActions, MatButton, MatDialogClose, AsyncPipe]
imports: [
MatDialogTitle,
CdkScrollable,
MatDialogContent,
MatDialogActions,
MatButton,
MatDialogClose,
AsyncPipe,
MatIcon
]
})
export class GenericDialogComponent<T> {
constructor(@Inject(MAT_DIALOG_DATA) private readonly data: GenericDialogOptions<T>) {}
Expand All @@ -46,11 +57,7 @@ export class GenericDialogComponent<T> {
return this.data.message;
}

get options(): {
label: Observable<string>;
value: T;
highlight?: boolean | undefined;
}[] {
get options(): GenericDialogOptions<T>['options'] {
return this.data.options;
}
}
Loading