Feature/configure sources #14
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 128 files out of 237 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThis pull request refines data types and expands functionality related to language handling across the application. The changes replace the older project type with a new type that includes a language tag, update method signatures and mocks, and adjust the API response to include language information. Additionally, new functions for language code processing, a draft sources component with its supporting files (HTML, SCSS, tests, Storybook stories), and a new route for accessing the draft sources feature have been introduced. Documentation has also been updated to clarify certain properties. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DS as DraftSourcesComponent
participant PS as ParatextService
participant API as Backend API
U->>DS: Navigate to Draft Sources page
DS->>PS: Request resources (with languageTag)
PS->>API: Fetch selectable projects data
API-->>PS: Return resources with languageTag
PS-->>DS: Return mapped resources
U->>DS: Select project and configure draft sources
DS->>DS: Validate selections and language codes
U->>DS: Trigger save action
DS->>API: Save draft sources configuration
API-->>DS: Return success/failure status
DS-->>U: Display update status
sequenceDiagram
participant S as SyncComponent
participant PD as ProjectData
participant F as isSFProjectSyncing
S->>PD: Retrieve project sync data (queuedCount)
S->>F: Evaluate sync status
F-->>S: Return boolean (active or not)
S->>S: Update _syncActive property based on result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/SIL.XForge.Scripture/Controllers/ParatextController.cs (1)
207-216: 🛠️ Refactor suggestionUpdate XML documentation to reflect the new return value structure.
The documentation needs to be updated to mention that the returned array now includes the language tag.
Apply this diff to update the documentation:
/// <summary> /// Retrieves the Paratext resources the user has access to. /// </summary> /// <response code="200"> /// The resources were successfully retrieved. A dictionary is returned where the Paratext Id is the key, and the - /// values are an array with the short name followed by the name. + /// values are an array containing: [shortName, name, languageTag]. /// </response> /// <response code="204">The user does not have permission to access Paratext.</response> /// <response code="401">The user's Paratext tokens have expired, and the user must log in again.</response> /// <response code="503">The Paratext registry is unavailable.</response>
🧹 Nitpick comments (10)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts (4)
9-9: Use the appropriate module import for progress spinner.Importing
MatProgressSpinnerdirectly from@angular/material/progress-spinneris unusual; typically we import and declareMatProgressSpinnerModulein the component module declarations. If you intended to use the component in the template, consider replacing this import with:- import { MatProgressSpinner } from '@angular/material/progress-spinner'; + import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
195-197: Revisit handling of equivalent language codes.A FIXME note indicates that language codes that differ only in casing or minor variations might need canonicalization. Consider a utility function or library to normalize language tags to prevent duplicates.
230-231: Address the "no actual nullish elements" FIXME.The filter logic removes nullish elements, but TypeScript can’t infer that these elements never become null. Consider refining the type definition or using a type guard to avoid forced casts.
Would you like a helper function or updated type definition to clarify that these arrays won't contain null?
248-249: Confirm loadProjects call location.
this.loadProjects()is invoked in the constructor. You might consider calling it fromngOnInit()to keep side effects (like data fetching) outside the constructor. This improves testability and aligns with Angular best practices.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts (1)
88-105: Remove commented-out code.The commented-out render function should be removed as it's no longer needed.
- // render: args => { - // // setUpMocks(args as DraftSourcesComponentStoryState); - // return { - // // moduleMetadata: { - // // providers: [ - // // { provide: ActivatedProjectService, useValue: instance(mockActivatedProjectService) }, - // // { provide: DestroyRef, useValue: instance(mockDestroyRef) }, - // // { provide: ParatextService, useValue: instance(mockParatextService) }, - // // { provide: DialogService, useValue: instance(mockDialogService) }, - // // { provide: SFProjectService, useValue: instance(mockProjectService) }, - // // { provide: SFUserProjectsService, useValue: instance(mockUserProjectsService) }, - // // { provide: Router, useValue: instance(mockRouter) }, - // // { provide: FeatureFlagService, useValue: instance(mockFeatureFlags) } - // // ] - // // }, - // template: `<app-draft-sources></app-draft-sources>` - // }; - // },src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources.service.ts (1)
223-225: Update XML documentation to include language tag.The XML documentation at line 211-213 should be updated to mention that the returned array now includes the language tag as the third element.
Apply this diff to update the documentation:
/// <response code="200"> /// The resources were successfully retrieved. A dictionary is returned where the Paratext Id is the key, and the - /// values are an array with the short name followed by the name. + /// values are an array with the short name, name, and language tag. /// </response>src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts (1)
65-75: Add test for save with invalid language codes.The test suite covers successful save and error cases, but it's missing a test for saving when
languageCodesConfirmedis false.Consider adding this test case:
it('should not save when language codes are not confirmed', fakeAsync(() => { const env = new TestEnvironment(); env.component.languageCodesConfirmed = false; env.component.save(); tick(); verify(mockedSFProjectService.onlineUpdateSettings(anything(), anything())).never(); }));src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.scss (1)
4-5: Consider moving color variables to a shared file.The color variables
$active-colorand$active-hover-colorcould be moved to the shared variables file (src/variables.scss) for better maintainability and consistency across components.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html (2)
1-1: Move hardcoded strings to translation files.Several strings are hardcoded in the template. Consider moving them to translation files for better internationalization support:
- "Configure draft sources"
- "Draft source"
- "Reference projects"
- "Target language data"
Also applies to: 8-9, 29-30, 72-73
6-25: Add accessibility attributes to improve user experience.The stepper-like interface could benefit from improved accessibility:
- Add
aria-labelto step headers- Add
aria-expandedto step bodies- Add
aria-currentto indicate the active stepExample:
-<div class="step" [class.active]="step === 1"> +<div class="step" [class.active]="step === 1" role="region" [attr.aria-label]="'Step 1: Draft source'"> - <div class="step-header" matRipple (click)="goToStep(1)"> + <div class="step-header" matRipple (click)="goToStep(1)" role="button" [attr.aria-expanded]="step === 1" [attr.aria-current]="step === 1 ? 'step' : false">Also applies to: 27-68, 70-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/SIL.XForge.Scripture/ClientApp/src/app/connect-project/connect-project.component.spec.ts(3 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/core/paratext.service.ts(3 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.spec.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync.component.ts(3 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources.service.ts(3 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.scss(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-add-resource-dialog/editor-tab-add-resource-dialog.service.spec.ts(2 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts(2 hunks)src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts(2 hunks)src/SIL.XForge.Scripture/Controllers/ParatextController.cs(1 hunks)test/SIL.XForge.Scripture.Tests/Controllers/ParatextControllerTests.cs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts
- src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.spec.ts
🔇 Additional comments (17)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts (1)
351-361: Enforce selection of at least one source and reference.These validations are crucial for ensuring project configuration correctness. Ensure you have adequate test coverage (unit or e2e) to verify that these prompts display and block saving appropriately.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts (2)
5-5: Good import organization.Importing the
DraftSourcesComponentat this line is straightforward and consistent with Angular’s recommended patterns.
21-25: Route configuration looks correct.The new route for
draft-sourcesappears properly configured with the guard. It cleanly extends the existing route structure under/projects/:projectId/.src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-add-resource-dialog/editor-tab-add-resource-dialog.service.spec.ts (1)
15-17: Verify consistency of resource identifiers.You've switched to using
SelectableProjectWithLanguageCode, which typically includes aparatextIdproperty in other files. Here you useidinstead. If this service ultimately relies onparatextIddownstream, ensure that field alignment is correct or consider renamingidtoparatextId.src/SIL.XForge.Scripture/ClientApp/src/app/core/paratext.service.ts (3)
33-42: LGTM! Well-documented interface.The JSDoc comments clearly explain the purpose and contents of the
SelectableProjectinterface.
44-46: LGTM! Clean interface extension.The
SelectableProjectWithLanguageCodeinterface properly extendsSelectableProjectwith the newlanguageTagproperty.
82-99: LGTM! Clean implementation.The
getResourcesmethod correctly maps the API response to include thelanguageTagproperty.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts (2)
44-48: LGTM! Good i18n support.The language code setup uses the Intl API for proper language name localization.
50-65: LGTM! Comprehensive test data.The mock data setup provides a good variety of test cases with realistic language-specific resources and projects.
src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync.component.ts (2)
19-22: LGTM! Well-documented utility function.The
isSFProjectSyncingfunction is clearly documented and encapsulates the sync status check logic for better reusability.
217-217: LGTM! Good refactoring.The direct check is replaced with the utility function, improving code maintainability.
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts (1)
96-96: LGTM! Improved type safety.The value setter now explicitly handles undefined values, making the component more robust.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources.service.ts (2)
30-33: LGTM!The interface is well-defined and properly documented. The extension of
SFProjectProfilewith aprojectRefproperty is appropriate for the use case.
148-153: LGTM!The change improves readability by using a named variable while maintaining type safety.
test/SIL.XForge.Scripture.Tests/Controllers/ParatextControllerTests.cs (1)
352-354: LGTM!The changes improve test reliability by using
ElementAtfor explicit access and properly verify the new language tag property.src/SIL.XForge.Scripture/ClientApp/src/app/connect-project/connect-project.component.spec.ts (1)
27-27: LGTM! Type changes and test data updates are consistent.The changes correctly update the type of
resourcesarray toSelectableProjectWithLanguageCodeand include the requiredlanguageTagproperty in the test data.Also applies to: 334-347
src/SIL.XForge.Scripture/Controllers/ParatextController.cs (1)
223-225: Verify client compatibility with the new array structure.The changes look good, but adding
LanguageTagto the array is a breaking change. Ensure that all clients are updated to handle the new array structure with three elements instead of two.Run the following script to find all usages of this endpoint:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Find all usages of the resources endpoint to verify client compatibility # Search for HTTP requests to the resources endpoint rg -A 5 "paratext-api/resources" # Search for array destructuring of resource values that might break ast-grep --pattern 'const [$_,$_] = $_'Length of output: 3747
I'll run an additional search to check if the client uses the data returned by the
/resourcesendpoint via any references to "ParatextId" or similar keywords:
🏁 Script executed:
#!/bin/bash # Searching for client references to 'ParatextId' in the repository rg "ParatextId" src/SIL.XForge.Scripture/ClientAppLength of output: 25304
Client API Compatibility Check: Array Structure Updated to Three Elements
I couldn’t locate any client-side code that explicitly assumes a two-element array from the
/resourcesendpoint. In our searches, no destructuring or indexing patterns were found that indicate the client expects only[ShortName, Name]. However, since this change is potentially breaking, please ensure that integration and manual end-to-end tests are run to confirm that all consumers handle the additionalLanguageTagelement without issues.
| MatProgressSpinner | ||
| ], | ||
| templateUrl: './draft-sources.component.html', | ||
| styleUrl: './draft-sources.component.scss' |
There was a problem hiding this comment.
Potential incorrect property name in the @component decorator.
Angular requires a "styleUrls" property (plural) for CSS/SCSS styling arrays rather than a single "styleUrl". This may cause a build error.
- styleUrl: './draft-sources.component.scss'
+ styleUrls: ['./draft-sources.component.scss']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| styleUrl: './draft-sources.component.scss' | |
| styleUrls: ['./draft-sources.component.scss'] |
|
|
||
| @if (showSourceAndTargetLanguagesIdenticalWarning) { | ||
| <app-notice mode="fill-dark" type="warning"> | ||
| <h3>Source and target languages are both {{ i18n.getLanguageDisplayName(targetLanguageTag) }}</h3> |
There was a problem hiding this comment.
Sanitize language display names to prevent XSS.
The language display name is directly interpolated into the template, which could be a security risk if the name contains malicious HTML/JavaScript. Use the Angular DomSanitizer to sanitize the value.
Example:
-<h3>Source and target languages are both {{ i18n.getLanguageDisplayName(targetLanguageTag) }}</h3>
+<h3>Source and target languages are both {{ i18n.getLanguageDisplayName(targetLanguageTag) | sanitize }}</h3>Add the sanitize pipe to your module:
import { DomSanitizer, SafeHtml } from '@angular/platform-browser';
import { Pipe, PipeTransform } from '@angular/core';
@Pipe({ name: 'sanitize' })
export class SanitizePipe implements PipeTransform {
constructor(private sanitizer: DomSanitizer) {}
transform(value: string): SafeHtml {
return this.sanitizer.sanitize(SecurityContext.HTML, value);
}
}Also applies to: 177-177, 178-178, 179-179, 180-180, 181-181
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts (2)
300-306: Avoid overshadowing theindexvariable
Inside thesourceSelectedmethod, this line redeclares anindexvariable that already exists as a parameter, which can be confusing. Consider using a different local variable name (e.g.,nullIndexorblankIndex).- const index = array.findIndex(s => s == null); - array.splice(index, 1); + const nullIndex = array.findIndex(s => s == null); + array.splice(nullIndex, 1);
358-358: Clarify or address the TODO
There is a TODO comment for verifying at least one source and reference is selected, but the validation logic seems partially implemented immediately below. Consider removing or updating this TODO to reflect its current status or open a separate task if further validation is needed.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts (1)
32-46: Consider extending test coverage
Most tests focus on the save logic. Consider adding tests for navigation steps (goToStep,cancel), manual project selection (sourceSelected), and language confirmation toggling (confirmationChanged). This would further solidify confidence in each user interaction path.src/SIL.XForge.Scripture/ClientApp/biome.json (1)
8-11: Consider adding common ignore patterns.An empty ignore list might lead to processing unnecessary files. Consider adding common patterns like
node_modules,dist,coverage, etc."files": { "ignoreUnknown": false, - "ignore": [] + "ignore": [ + "**/node_modules/**", + "**/dist/**", + "**/coverage/**", + "**/.git/**" + ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/SIL.XForge.Scripture/ClientApp/biome.json(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts(1 hunks)src/SIL.XForge.Scripture/Controllers/ParatextController.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/SIL.XForge.Scripture/Controllers/ParatextController.cs
- src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html
- src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts
🔇 Additional comments (6)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts (1)
120-120: Property name should bestyleUrls
This property name is missing the "s" at the end, which can cause a build error in Angular.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts (1)
1-4: Ensure browser compatibility withIntl.DisplayNames
Intl.LocaleandIntl.DisplayNamesare relatively new APIs and may require polyfills in older browsers. Confirm that your environment or build configuration provides the necessary fallback or mark the required browser support.src/SIL.XForge.Scripture/ClientApp/biome.json (4)
12-16: LGTM!The formatter settings follow common JavaScript/TypeScript conventions with 2-space indentation.
17-19: LGTM!Import organization is properly enabled for automatic import sorting.
30-37: LGTM!The JavaScript-specific formatting settings are well-configured with reasonable defaults:
- Single quotes for consistency
- No trailing commas for cleaner git diffs
- 120-character line width balances readability and screen space
- Minimal arrow function parentheses reduces visual noise
20-29:❓ Verification inconclusive
Verify linter rule configuration.
The configuration disables two style rules:
useImportType: Allows type imports to be omittednoInferrableTypes: Allows explicit type annotations even when they can be inferredPlease confirm if disabling these rules aligns with the team's TypeScript coding guidelines.
🏁 Script executed:
#!/bin/bash # Check for existing TypeScript configurations that might conflict fd -e json -e yaml -e yml . | xargs grep -l "useImportType\|noInferrableTypes"Length of output: 137
Action Required: Confirm Linter Rule Settings
The linter configuration in
src/SIL.XForge.Scripture/ClientApp/biome.jsondisables the following style rules:
useImportType— allows omission of type importsnoInferrableTypes— permits explicit type annotations even when inferableA repository search revealed that these settings are defined in both:
biome.jsonsrc/SIL.XForge.Scripture/ClientApp/biome.jsonPlease verify that:
- Disabling these two rules is intentional for the project's TypeScript guidelines.
- The presence of these settings in multiple configuration files is expected or if they should be consolidated.
| "vcs": { | ||
| "enabled": false, | ||
| "clientKind": "git", | ||
| "useIgnoreFile": false | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify VCS configuration.
The VCS settings appear contradictory - clientKind is set to "git" but VCS is disabled. This might prevent Git-related features from working correctly.
🌐 Web query:
What are the implications of disabling VCS in Biome while specifying git as clientKind?
💡 Result:
Disabling VCS integration in Biome (vcs.enabled: false) while specifying git as the clientKind creates a configuration mismatch with these implications:
1. VCS Features Become Inactive
- Biome will ignore all Git-related functionality despite
clientKind: "git"[1][3]. This includes:- Ignoring
.gitignorefiles (even ifvcs.useIgnoreFile: true)[1][6] - Disabled staged/changed file detection (
--staged/--changedCLI flags won’t work)[3][4] - No branch-based operations (like comparing against
vcs.defaultBranch)[3]
- Ignoring
2. Configuration Redundancy
- The
clientKind: "git"setting becomes non-functional since VCS integration is disabled[1][3]. This creates a "dead configuration" scenario where settings likevcs.rootorvcs.defaultBranchhave no effect.
3. Behavioral Impact
| Feature | Enabled (vcs.enabled: true) |
Disabled (vcs.enabled: false) |
|---|---|---|
.gitignore handling |
Honors ignore rules[1][3] | Ignores all Git paths[6] |
| Staged file checks | Works with --staged[3] |
Fails silently[3] |
| Changed file detection | Works with --changed[3] |
Unavailable[3] |
4. Diagnostic Warnings
Biome may emit warnings about unused VCS configurations when enabled: false but other VCS properties are defined[1][7].
Practical Example
This configuration creates a contradiction:
{
"vcs": {
"enabled": false,
"clientKind": "git",
"useIgnoreFile": true
}
}Despite specifying Git integration details:
- Files matching
.gitignorepatterns will not be ignored[6] - Running
biome check --stagedbecomes equivalent to a regular check[3] - Default branch settings are disregarded[3]
To resolve this mismatch:
- Either enable VCS (
vcs.enabled: true) for full Git integration - Or remove all
vcs.*properties if Git features aren’t needed[1][7]
Citations:
- 1: https://biomejs.dev/reference/configuration/
- 2: https://blog.serverlessadvocate.com/how-to-use-biome-in-aws-cdk-solutions-44671ae9a3a4
- 3: https://biomejs.dev/guides/integrate-in-vcs/
- 4: https://www.biomejs.cn/blog/
- 5: https://blog.tericcabrel.com/nodejs-typescript-biome/
- 6:
vcs.useIgnoreFileGit integration does not take.git/info/excludeinto account biomejs/biome#4822 - 7: https://biomejs.dev/linter/
- 8: https://kittygiraudel.com/2024/06/01/from-eslint-and-prettier-to-biome/
- 9: 📝 New lines are added after formatting biomejs/biome#2877
- 10: 🐛 VCS ignore path error biomejs/biome#709
Action Required: Resolve VCS Configuration Mismatch
The Biome configuration in src/SIL.XForge.Scripture/ClientApp/biome.json has a conflicting setup: while clientKind is set to "git", VCS integration is disabled (enabled: false). As the web documentation indicates, disabling VCS means that any Git-related functionality (like honoring .gitignore rules or detecting staged changes) won’t be active—even though Git is specified as the client. This results in redundant, non-functional settings.
Recommended Actions:
- If Git integration is desired: Set
"enabled": trueto allow Biome to utilize Git features. - If Git integration is not required: Remove the Git-specific properties (e.g.,
"clientKind": "git") to avoid misleading configuration.
Nateowami
force-pushed
the
feature/configure-sources
branch
from
57f317a to
7bc01ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts (4)
29-37: Add documentation for the mixedSource property.The purpose of the
mixedSourceboolean flag in theDraftSourcesComponentStoryStateinterface is not immediately clear from its name. Consider adding JSDoc comments to explain its purpose and impact on the component's behavior.
44-49: Consider moving language codes to a configuration file.The hardcoded language codes could be moved to a shared configuration file for better maintainability and reusability across the application.
-const languageCodes = ['en', 'fr', 'es', 'pt', 'de', 'ru', 'zh', 'ar', 'hi', 'bn']; +import { SUPPORTED_LANGUAGE_CODES } from '../../../core/config/language-codes'; + +const languageCodes = SUPPORTED_LANGUAGE_CODES;
70-95: Remove empty argTypes object.The empty
argTypesobject can be safely removed as it doesn't provide any configuration.export default { title: 'Draft/DraftSources', component: DraftSourcesComponent, decorators: [...], args: defaultArgs, parameters: { controls: { include: Object.keys(defaultArgs) } }, - argTypes: {} } as Meta<DraftSourcesComponentStoryState>;
97-108: Remove empty args from Default story.The empty
argsobject in the Default story might override the default args. Consider removing it if you want to use the default args defined earlier.export const Default: Story = { - ...Template, - args: {} + ...Template };src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts (3)
57-63: Consider adding more initialization test cases.The initialization test verifies basic loading but could be expanded to cover error scenarios and edge cases.
Add test cases for:
- Failed project loading
- Empty project/resource lists
- Network errors during initialization
152-306: Add boundary tests for sourceArraysToSettingsChange.While the test coverage is good, consider adding boundary tests.
Add test cases for:
- Maximum array length boundaries
- Invalid project IDs
- Malformed project objects
310-360: Enhance TestEnvironment for better reusability.The TestEnvironment class could be more flexible and reusable.
Consider:
- Adding factory methods for different test scenarios
- Making mock data configurable through constructor parameters
- Adding helper methods for common test operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts(1 hunks)src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts(1 hunks)src/SIL.XForge.Scripture/Controllers/ParatextController.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SIL.XForge.Scripture/Controllers/ParatextController.cs
- src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts
🔇 Additional comments (5)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts (3)
1-28: LGTM! Well-structured mock setup.The imports are logically organized, and all necessary services are properly mocked using ts-mockito.
39-43: LGTM! Clear and concise test setup.The test project configuration and feature flag setup follow testing best practices.
50-69: LGTM! Efficient and consistent mock data generation.The mock data generation is well-structured, producing realistic test data that matches the expected interfaces.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts (1)
120-120: Fix styleUrl property in @component decorator.The
styleUrlproperty should bestyleUrls(plural) for Angular component decorators.- styleUrl: './draft-sources.component.scss' + styleUrls: ['./draft-sources.component.scss']src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html (1)
176-176: Sanitize language display names to prevent XSS.Direct interpolation of language display names could be a security risk.
Use Angular's DomSanitizer to prevent XSS attacks.
| async save(): Promise<void> { | ||
| const definedSources = this.draftingSources.filter(s => s != null); | ||
| const definedReferences = this.trainingSources.filter(s => s != null); | ||
|
|
||
| let message: string | undefined; | ||
| if (definedSources.length === 0 && definedReferences.length === 0) | ||
| message = 'Please select at least one source and one reference project before saving.'; | ||
| else if (definedSources.length === 0) message = 'Please select at least one source project before saving.'; | ||
| else if (definedReferences.length === 0) message = 'Please select at least one reference project before saving.'; | ||
| else if (!this.languageCodesConfirmed) | ||
| message = 'Please confirm that the language codes are correct before saving.'; | ||
|
|
||
| if (message) { | ||
| this.dialogService.message(of(message)); | ||
| return; | ||
| } | ||
|
|
||
| if (this.activatedProjectService.projectDoc == null) throw new Error('Project doc is null'); | ||
| if (this.activatedProjectService.projectDoc.data == null) throw new Error('Project doc data is null'); | ||
| const currentProjectParatextId: string = this.activatedProjectService.projectDoc.data.paratextId; | ||
| const sourcesSettingsChange: DraftSourcesSettingsChange = sourceArraysToSettingsChange( | ||
| this.trainingSources, | ||
| this.draftingSources, | ||
| this.trainingTargets, | ||
| currentProjectParatextId | ||
| ); | ||
| const projectSettingsChange: SFProjectSettings = sourcesSettingsChange; | ||
| const currentSFProjectId = this.activatedProjectService.projectId; | ||
| if (currentSFProjectId == null) throw new Error('Project ID is null'); | ||
| await this.checkUpdateStatus( | ||
| 'projectSettings', | ||
| this.projectService.onlineUpdateSettings(currentSFProjectId, projectSettingsChange) | ||
| ); | ||
| this.monitorSyncStatus(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in save method.
The error handling in the save method could be more robust.
Consider:
async save(): Promise<void> {
+ try {
const definedSources = this.draftingSources.filter(s => s != null);
const definedReferences = this.trainingSources.filter(s => s != null);
let message: string | undefined;
if (definedSources.length === 0 && definedReferences.length === 0)
message = 'Please select at least one source and one reference project before saving.';
else if (definedSources.length === 0) message = 'Please select at least one source project before saving.';
else if (definedReferences.length === 0) message = 'Please select at least one reference project before saving.';
else if (!this.languageCodesConfirmed)
message = 'Please confirm that the language codes are correct before saving.';
if (message) {
this.dialogService.message(of(message));
return;
}
if (this.activatedProjectService.projectDoc == null) throw new Error('Project doc is null');
if (this.activatedProjectService.projectDoc.data == null) throw new Error('Project doc data is null');
const currentProjectParatextId: string = this.activatedProjectService.projectDoc.data.paratextId;
const sourcesSettingsChange: DraftSourcesSettingsChange = sourceArraysToSettingsChange(
this.trainingSources,
this.draftingSources,
this.trainingTargets,
currentProjectParatextId
);
const projectSettingsChange: SFProjectSettings = sourcesSettingsChange;
const currentSFProjectId = this.activatedProjectService.projectId;
if (currentSFProjectId == null) throw new Error('Project ID is null');
await this.checkUpdateStatus(
'projectSettings',
this.projectService.onlineUpdateSettings(currentSFProjectId, projectSettingsChange)
);
this.monitorSyncStatus();
+ } catch (error) {
+ console.error('Failed to save draft sources:', error);
+ this.dialogService.message(of('Failed to save draft sources. Please try again.'));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async save(): Promise<void> { | |
| const definedSources = this.draftingSources.filter(s => s != null); | |
| const definedReferences = this.trainingSources.filter(s => s != null); | |
| let message: string | undefined; | |
| if (definedSources.length === 0 && definedReferences.length === 0) | |
| message = 'Please select at least one source and one reference project before saving.'; | |
| else if (definedSources.length === 0) message = 'Please select at least one source project before saving.'; | |
| else if (definedReferences.length === 0) message = 'Please select at least one reference project before saving.'; | |
| else if (!this.languageCodesConfirmed) | |
| message = 'Please confirm that the language codes are correct before saving.'; | |
| if (message) { | |
| this.dialogService.message(of(message)); | |
| return; | |
| } | |
| if (this.activatedProjectService.projectDoc == null) throw new Error('Project doc is null'); | |
| if (this.activatedProjectService.projectDoc.data == null) throw new Error('Project doc data is null'); | |
| const currentProjectParatextId: string = this.activatedProjectService.projectDoc.data.paratextId; | |
| const sourcesSettingsChange: DraftSourcesSettingsChange = sourceArraysToSettingsChange( | |
| this.trainingSources, | |
| this.draftingSources, | |
| this.trainingTargets, | |
| currentProjectParatextId | |
| ); | |
| const projectSettingsChange: SFProjectSettings = sourcesSettingsChange; | |
| const currentSFProjectId = this.activatedProjectService.projectId; | |
| if (currentSFProjectId == null) throw new Error('Project ID is null'); | |
| await this.checkUpdateStatus( | |
| 'projectSettings', | |
| this.projectService.onlineUpdateSettings(currentSFProjectId, projectSettingsChange) | |
| ); | |
| this.monitorSyncStatus(); | |
| } | |
| async save(): Promise<void> { | |
| + try { | |
| const definedSources = this.draftingSources.filter(s => s != null); | |
| const definedReferences = this.trainingSources.filter(s => s != null); | |
| let message: string | undefined; | |
| if (definedSources.length === 0 && definedReferences.length === 0) | |
| message = 'Please select at least one source and one reference project before saving.'; | |
| else if (definedSources.length === 0) message = 'Please select at least one source project before saving.'; | |
| else if (definedReferences.length === 0) message = 'Please select at least one reference project before saving.'; | |
| else if (!this.languageCodesConfirmed) | |
| message = 'Please confirm that the language codes are correct before saving.'; | |
| if (message) { | |
| this.dialogService.message(of(message)); | |
| return; | |
| } | |
| if (this.activatedProjectService.projectDoc == null) throw new Error('Project doc is null'); | |
| if (this.activatedProjectService.projectDoc.data == null) throw new Error('Project doc data is null'); | |
| const currentProjectParatextId: string = this.activatedProjectService.projectDoc.data.paratextId; | |
| const sourcesSettingsChange: DraftSourcesSettingsChange = sourceArraysToSettingsChange( | |
| this.trainingSources, | |
| this.draftingSources, | |
| this.trainingTargets, | |
| currentProjectParatextId | |
| ); | |
| const projectSettingsChange: SFProjectSettings = sourcesSettingsChange; | |
| const currentSFProjectId = this.activatedProjectService.projectId; | |
| if (currentSFProjectId == null) throw new Error('Project ID is null'); | |
| await this.checkUpdateStatus( | |
| 'projectSettings', | |
| this.projectService.onlineUpdateSettings(currentSFProjectId, projectSettingsChange) | |
| ); | |
| this.monitorSyncStatus(); | |
| + } catch (error) { | |
| + console.error('Failed to save draft sources:', error); | |
| + this.dialogService.message(of('Failed to save draft sources. Please try again.')); | |
| + } | |
| } |
| <div class="component-footer"> | ||
| <div class="page-actions"> | ||
| <button mat-button (click)="cancel()"><mat-icon>close</mat-icon> Cancel</button> | ||
| <button mat-flat-button color="primary" (click)="save()"><mat-icon>check</mat-icon> Save & sync</button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation dialog for unsaved changes.
The cancel button should prompt for confirmation if there are unsaved changes.
- <button mat-button (click)="cancel()"><mat-icon>close</mat-icon> Cancel</button>
+ <button mat-button (click)="confirmCancel()"><mat-icon>close</mat-icon> Cancel</button>Add to component:
async confirmCancel(): Promise<void> {
if (!this.changesMade || await this.dialogService.confirm(
'You have unsaved changes. Are you sure you want to cancel?',
'Yes, discard changes',
'No, keep editing'
)) {
this.cancel();
}
}
Co-authored-by: Nateowami <6140710+Nateowami@users.noreply.github.com>
This reverts commit 6947dbe.
Co-authored-by: Nateowami <6140710+Nateowami@users.noreply.github.com>
rade Storybook to work with updated dependencies
Based on some discussion of the philosophy of testing this component at this time. The 'should not save when language codes are not confirmed' test would be presumably passing for the wrong reason at this time.
…hat are no longer available
Nateowami
force-pushed
the
feature/configure-sources
branch
from
7bc01ef to
2446a32
Compare
Summary by CodeRabbit
New Features
Improvements