test: add duplication test #2309
Conversation
WalkthroughThis change set primarily introduces new and refactored test utilities for frontend Playwright-based UI testing, including new page object models for forms, tables, rows, modals, and list views. Several frontend components and test files update their Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Browser
participant LoginPage
participant ListViewPage
participant DetailViewPage
participant CreateModal
participant ModelForm
Tester->>Browser: Launch test
Tester->>LoginPage: doLoginP()
LoginPage->>Browser: Fill credentials, submit
Tester->>ListViewPage: getOpenCreateModal()
ListViewPage->>CreateModal: Open modal
CreateModal->>ModelForm: getForm()
Tester->>ModelForm: doFillForm(data)
ModelForm->>Browser: Fill fields, select autocomplete, submit
Tester->>ListViewPage: doSearch()
ListViewPage->>Browser: Search for created object
Tester->>ListViewPage: getFirstRow()
ListViewPage->>DetailViewPage: gotoDetailView()
DetailViewPage->>CreateModal: getOpenDuplicateModal()
CreateModal->>ModelForm: getForm()
Tester->>ModelForm: doFillForm(modifiedData)
ModelForm->>Browser: Fill fields, submit
Tester->>ListViewPage: doSearch()
ListViewPage->>Browser: Verify duplicated object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
monsieurswag
force-pushed
the
refactor_user_routes_test
branch
from
e89071e to
3efac37
Compare
Remove some more useless stuff Remove useless comment 2 Remove useless comment
monsieurswag
force-pushed
the
refactor_user_routes_test
branch
from
a11c409 to
e6e653d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
frontend/src/lib/components/Forms/AutocompleteSelect.svelte (1)
31-34: Duplicatedisabledproperty inProps– pick one
disabled?: boolean;is declared twice in thePropsinterface (Line 33 and Line 62).
Type-script will merge them, but the duplication is noisy and risks the two positions diverging (e.g. different JSDoc). Drop the second occurrence.@@ interface Props { cachedValue?: any[] | undefined; - disabled?: boolean; mount?: (value: any) => void; }Also applies to: 60-63
frontend/tests/utilsv2/base/model-table.ts (1)
39-48: Implement or remove the unimplemented method.The
getRowmethod throws an error when called, which could cause issues if accidentally invoked. Consider either:
- Implementing the method if it's needed for current functionality
- Removing it if it's not currently required
- Making it protected with a clear comment about future implementation
If implementing, here's a suggested approach:
- getRow(index?: number) { - // TODO: Implement row retrieval logic - throw new Error('Method not implemented'); - } + getRow(index: number = 0): ModelTableRow { + return this._getSubElement(ModelTableRow, { + nth: index + }); + }frontend/tests/utilsv2/core/element.ts (1)
172-182: Remove duplicate error handling code.There are two identical error checks for missing
DATA_TESTID. The second one (lines 178-182) is unreachable and should be removed.const dataTestId = filters.dataTestId || elementClass.DATA_TESTID; if (!dataTestId) throw new Error( `Both the DATA_TESTID (${elementClass.DATA_TESTID}) property and the dataTestId filter (${filters.dataTestId}) are falsy evaluated.` ); - // Should i really check if this property is undefined ? - // After all typescript should reveal if the class is missing this property when using this method. - if (!dataTestId) { - throw new Error( - `Missing required static string property 'DATA_TESTID' in class '${elementClass.name}'.` - ); - }
🧹 Nitpick comments (7)
frontend/tests/utilsv2/core/base.ts (1)
18-18: Clean up the stale TODO & typo
// The Fixture interace …
• “interace” → “interface”.
• If the fixtures &testoverride are already implemented elsewhere, delete this comment; otherwise, track it with an issue instead of leaving TODOs in core code.frontend/tests/utilsv2/core/page.ts (1)
75-78: Expose load-wait flexibility & allow chaining
waitUntilLoaded()hard-codes'load'. In many SPA routes'networkidle'or'domcontentloaded'is faster/more reliable.-async waitUntilLoaded() { - await this._self.waitForLoadState('load'); -} +async waitUntilLoaded(state: 'load' | 'domcontentloaded' | 'networkidle' = 'load') { + await this._self.waitForLoadState(state); + return this as this; // enables fluent chaining +}Optional, but it avoids magic constants and supports fluent APIs.
frontend/tests/functional/object-duplication.test.ts (1)
125-159: Comprehensive test implementation with good practices.The main test loop effectively covers the complete object duplication workflow. The use of
structuredCloneprevents data mutation between iterations, and the test properly verifies each step of the duplication process. The commented line at 154 could be removed if not needed.Consider removing the commented line 154 if it's not needed:
- // await appliedControlListView.waitUntilLoaded();frontend/tests/utilsv2/base/autocomplete-select-input.ts (1)
8-8: Consider using a more descriptive approach for dynamic test IDs.Setting
DATA_TESTID = undefinedis unusual. Consider documenting why this component requires dynamic data-testid handling or providing a more explicit pattern.- static DATA_TESTID = undefined; // Dynamic data-testid. + static DATA_TESTID: string | undefined = undefined; // Dynamic data-testid - must be provided via filtersfrontend/tests/utilsv2/derived/create-modal.ts (1)
25-101: Consider using a generic approach to reduce code duplication.All modal classes follow the identical pattern. While type safety is important, consider if a more generic approach could reduce this boilerplate code.
The current approach ensures type safety but creates significant code duplication. This is acceptable for test utilities where clarity and explicit typing are often preferred over DRY principles. The pattern is consistent and maintainable.
frontend/tests/utilsv2/derived/model-form.ts (2)
87-87: Update JSDoc comment to reflect actual implementation.The comment states "This function doesn't support the
folderargument" but the implementation does handle thefolderfield conditionally on line 92.- /** This function doesn't support the `folder` argument. */ + /** Fills the perimeter form with provided data. The folder field is optional. */
154-154: Consider using composition over aliasing for form reuse.Using a type alias for
AppliedControlDuplicateFormmay cause confusion if the forms need to diverge in behavior later.Consider creating a proper class that extends or composes the base form:
-export const AppliedControlDuplicateForm = AppliedControlCreateForm; +export class AppliedControlDuplicateForm extends AppliedControlCreateForm { + // Can add duplicate-specific behavior here if needed +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
frontend/src/lib/components/DetailView/DetailView.svelte(1 hunks)frontend/src/lib/components/Forms/AutocompleteSelect.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(1 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(2 hunks)frontend/src/lib/components/ModelTable/RowCount.svelte(1 hunks)frontend/src/lib/components/ModelTable/Search.svelte(1 hunks)frontend/src/lib/utils/schemas.ts(0 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts(0 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte(1 hunks)frontend/tests/functional/domain-import.test.ts(1 hunks)frontend/tests/functional/object-duplication.test.ts(1 hunks)frontend/tests/utilsv2/base/autocomplete-select-input.ts(1 hunks)frontend/tests/utilsv2/base/detail-view-page.ts(1 hunks)frontend/tests/utilsv2/base/list-view-page.ts(1 hunks)frontend/tests/utilsv2/base/model-form.ts(2 hunks)frontend/tests/utilsv2/base/model-table.ts(1 hunks)frontend/tests/utilsv2/core/base.ts(2 hunks)frontend/tests/utilsv2/core/element.ts(9 hunks)frontend/tests/utilsv2/core/fixtures.ts(2 hunks)frontend/tests/utilsv2/core/hot-reloader.ts(1 hunks)frontend/tests/utilsv2/core/page.ts(1 hunks)frontend/tests/utilsv2/derived/create-modal.ts(1 hunks)frontend/tests/utilsv2/derived/list-view.ts(1 hunks)frontend/tests/utilsv2/derived/login-page.ts(1 hunks)frontend/tests/utilsv2/derived/model-form.ts(1 hunks)frontend/tests/utilsv2/derived/model-table-filter.ts(1 hunks)frontend/tests/utilsv2/derived/model-table-footer.ts(1 hunks)frontend/tests/utilsv2/derived/model-table-row.ts(1 hunks)frontend/tests/utilsv2/derived/toast.ts(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts
- frontend/src/lib/utils/schemas.ts
🧰 Additional context used
🧠 Learnings (2)
frontend/src/lib/components/Forms/ModelForm.svelte (1)
Learnt from: nas-tabchiche
PR: #1486
File: frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte:0-0
Timestamp: 2025-02-13T11:07:39.687Z
Learning: Form validation constraints for components should be defined in zod schemas located in $lib/schemas.ts rather than directly in the form components.
frontend/tests/utilsv2/derived/model-form.ts (1)
Learnt from: nas-tabchiche
PR: #1486
File: frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte:0-0
Timestamp: 2025-02-13T11:07:39.687Z
Learning: Form validation constraints for components should be defined in zod schemas located in $lib/schemas.ts rather than directly in the form components.
🧬 Code Graph Analysis (8)
frontend/tests/utilsv2/core/hot-reloader.ts (1)
frontend/src/lib/utils/i18n.ts (1)
safeTranslate(88-90)
frontend/tests/utilsv2/base/list-view-page.ts (1)
frontend/tests/utilsv2/base/model-table.ts (1)
ModelTable(5-49)
frontend/tests/utilsv2/core/fixtures.ts (1)
frontend/tests/utilsv2/derived/login-page.ts (1)
LoginPage(7-40)
frontend/tests/functional/object-duplication.test.ts (9)
frontend/tests/utilsv2/core/page.ts (2)
Page(7-88)Class(90-90)frontend/tests/utilsv2/core/element.ts (2)
Class(264-264)Element(5-255)frontend/tests/utilsv2/base/list-view-page.ts (1)
ListViewPage(9-37)frontend/tests/utilsv2/base/create-modal.ts (1)
CreateModal(6-12)frontend/tests/utilsv2/derived/list-view.ts (5)
AppliedControlListViewPage(55-63)RiskAssessmentListViewPage(95-103)LibraryListViewPage(145-184)FolderListViewPage(20-33)PerimeterListViewPage(35-43)frontend/tests/utilsv2/derived/create-modal.ts (2)
AppliedControlCreateModal(37-41)RiskAssessmentCreateModal(67-71)frontend/tests/utilsv2/core/hot-reloader.ts (1)
hotReloadLoop(374-403)frontend/tests/utilsv2/core/test-data.ts (2)
ADMIN_EMAIL(1-1)ADMIN_PASSWORD(2-2)frontend/src/lib/utils/i18n.ts (1)
safeTranslate(88-90)
frontend/tests/utilsv2/base/model-table.ts (4)
frontend/tests/utilsv2/core/element.ts (2)
Element(5-255)Args(258-258)frontend/tests/utils/test-utils.ts (1)
Locator(798-798)frontend/tests/utilsv2/core/base.ts (1)
expect(6-6)frontend/tests/utilsv2/derived/model-table-row.ts (1)
ModelTableRow(7-81)
frontend/tests/utilsv2/derived/model-table-footer.ts (1)
frontend/tests/utilsv2/core/element.ts (2)
Element(5-255)Args(258-258)
frontend/tests/utilsv2/base/model-form.ts (1)
frontend/tests/utilsv2/core/base.ts (2)
expect(6-6)notImplemented(14-16)
frontend/tests/utilsv2/core/element.ts (1)
frontend/tests/utilsv2/core/page.ts (2)
Page(7-88)Class(90-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
🔇 Additional comments (39)
frontend/src/lib/components/ModelTable/RowCount.svelte (1)
15-15:data-testidrename looks good – remember to update callersThe selector update is harmless for runtime code, but any downstream tests, page-objects, or helper utilities that still look for
"row-count"will now fail. Make sure all usages have been migrated (grep forrow-count").frontend/src/lib/components/Forms/AutocompleteSelect.svelte (1)
493-499: Spinner test-id updated, keep the CSS hook as fallback
data-testidchanged toautocomplete-select-loading-elem, while the CSS class is still.loading-spinner.
If any tests/scripts key off the class rather than the attribute, they will continue to pass – nice backward compatibility. No further action needed.frontend/src/lib/components/ModelTable/Search.svelte (1)
25-25: Search input test-id renamed – LGTMChange is self-contained. Ensure Playwright helpers use the new id.
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte (1)
245-246: Consistent test-id naming – LGTM
duplicate-button-elemaligns with the new convention. No behavioural impact.frontend/src/lib/components/Forms/ModelForm.svelte (1)
226-226: Newdata-testidattribute added – approvedAdds a stable selector to the root
<SuperForm>; valuable for E2E tests.frontend/src/lib/components/DetailView/DetailView.svelte (1)
519-523: Verify all usages after changingdata-testidThe selector was renamed to
duplicate-button-elem.
Please confirm that:
- All Playwright page-objects (
DetailViewPage, etc.) now use the new id.- No CI tests still expect the old
duplicate-button.frontend/tests/utilsv2/core/hot-reloader.ts (1)
11-13: Path-alias may break underevalexecution
$lib/utils/i18nrelies on Vite/SvelteKit alias resolution.
When the hot-reloader transpiles thenevals code inside Node, Node’s module loader cannot resolve this alias unlesstsconfigPathsor a custom loader is active.Double-check the test runner: if aliases are unresolved you’ll hit
Error: Cannot find module '$lib/utils/i18n'.frontend/src/lib/components/ModelTable/ModelTable.svelte (2)
386-387: 👍 Addition of table-leveldata-testidThe top-level id (
model-table) is clear and consistent with the new pattern. No issues noticed.
482-483: 👍 Row selector is consistent
model-table-row-elemmatches the convention adopted across the suite and will simplify row location in tests.frontend/tests/functional/domain-import.test.ts (1)
31-31: LGTM! Consistent data-testid update.The update from
'row-count'to'row-count-elem'aligns with the corresponding UI component changes and maintains test reliability.frontend/tests/utilsv2/base/list-view-page.ts (1)
23-25: LGTM! Enhanced test reliability with visibility filter.Adding the
{ visible: true }option ensures that only visibleModelTableinstances are retrieved, which improves test reliability and reduces potential flakiness from interactions with hidden elements.frontend/tests/utilsv2/derived/toast.ts (2)
4-4: LGTM! Good documentation addition.The JSDoc comment clearly describes the component representation, improving code documentation.
8-9: LGTM! Enhanced flexibility and proper async handling.Two excellent improvements:
- Supporting
string | RegExpmakes the method more flexible for text matching patterns- Adding
awaitensures proper asynchronous handling of Playwright expectationsfrontend/tests/utilsv2/derived/login-page.ts (1)
19-20: LGTM! Streamlined login input flow.Moving the fill operations to happen immediately after element retrieval reduces code duplication while maintaining the fallback mechanism. This makes the code more efficient and logically structured.
frontend/tests/utilsv2/core/fixtures.ts (3)
6-6: LGTM! Minor reordering for consistency.Reordering the
pageproperty declaration maintains interface consistency without affecting functionality.
15-15: LGTM! Cleaner type inference.Removing the explicit type annotation allows TypeScript to infer the type naturally, which is appropriate here and reduces verbosity.
17-17: LGTM! Simplified fixture creation.Directly passing the
LoginPageinstance touse()eliminates the unnecessary intermediate variable, making the code more concise.frontend/tests/utilsv2/base/model-form.ts (4)
3-3: LGTM!The addition of the
Expecttype import is necessary for the enhanceddoFillFormmethod signature.
14-16: Enhanced method signature improves testing capabilities.The addition of the
expectparameter enables assertions during form filling, which is essential for robust test validation. This enhancement allows derived classes to perform validations while interacting with form elements.
18-24: Excellent improvement for test reliability.The enhanced submit logic with force click and retry mechanism effectively handles potential timing issues and UI state changes that can cause flaky tests. The implementation is sound and should improve test stability.
26-33: Well-implemented async loading state handler.The
_waitLoadingSpinsmethod properly handles multiple loading spinners and ensures all are detached before proceeding. This is essential for preventing race conditions in tests involving autocomplete components with async loading states.frontend/tests/utilsv2/derived/model-table-footer.ts (1)
8-16: Implementation follows established patterns correctly.The class implementation properly extends
Element, uses appropriate test ID constants, and follows the constructor pattern used throughout the codebase. The_rowCounterlocator initialization is correct.frontend/tests/functional/object-duplication.test.ts (5)
1-23: Well-organized imports using the new test framework.The import structure is clean and comprehensive, properly utilizing the new v2 test utilities. The separation of type imports and the inclusion of i18n utilities shows good organization.
24-64: Well-structured test data with clear interface definition.The
LocalTestDatainterface andTEST_DATAarray provide a clean, type-safe approach to defining test scenarios. The constant definitions and descriptive naming make the test data easy to understand and maintain.
66-68: LGTM!The hot reload setup is clean and will improve development productivity for this test.
70-81: Proper test setup with good practices.The test initialization correctly marks the test as slow, follows proper login flow with necessary waits and checks, and properly handles the post-login modal closure.
82-123: Comprehensive prerequisite setup with proper verification.The prerequisite steps are well-organized and properly verified. The use of test steps with descriptive names and toast message verification ensures the test environment is correctly prepared for the main test scenarios.
frontend/tests/utilsv2/base/detail-view-page.ts (2)
7-13: LGTM!The constructor properly extends the
Pageclass and initializes the description list locator with the correct test ID.
15-34: Excellent implementation of value verification.The
checkValuesmethod efficiently handles parallel processing of description list entries and provides comprehensive error messaging. The use ofsafeTranslateensures proper i18n support, and the detailed error messages will help with debugging test failures.frontend/tests/utilsv2/base/model-table.ts (4)
2-3: Well-structured property additions and visibility improvements.The new locator properties use appropriate test IDs and the constructor properly initializes them. Changing
_columnNamesvisibility to protected improves extensibility for derived classes. The import ofModelTableRowsupports the new functionality.Also applies to: 7-16
18-25: LGTM!The search functionality methods are well-implemented with clear documentation and follow established patterns for test utility methods.
27-30: LGTM!The
checkDisplayedRowCountmethod correctly uses the model table row test ID and provides essential verification functionality for test assertions.
32-37: LGTM!The
getFirstRowmethod properly uses the_getSubElementpattern with appropriate filtering and returns the correct type. The documentation clearly indicates the test ID being used.frontend/tests/utilsv2/core/element.ts (2)
136-142: LGTM! Well-implemented navigation method.The
_gotomethod provides a clean way to navigate to different pages while maintaining the current page context. The implementation correctly instantiates the target page class with the current page's Playwright instance.
184-190: Good enhancement to filter capabilities.Adding support for the 'visible' filter extends the element filtering capabilities appropriately. The implementation correctly checks for the presence of filter properties before applying them.
frontend/tests/utilsv2/derived/model-form.ts (1)
46-397: Excellent form abstraction with consistent patterns.The form classes follow a well-structured pattern:
- Clear separation of concerns with private locators
- Consistent constructor initialization
- Uniform
doFillFormmethod signature- Proper handling of optional fields with nullish coalescing
- Good use of
_waitLoadingSpins()before autocomplete interactionsThis provides a solid foundation for UI testing automation.
frontend/tests/utilsv2/derived/list-view.ts (3)
35-143: Excellent consistent implementation across resource list views.All the new list view page classes follow an identical, well-structured pattern:
- Clear constructor with appropriate URL paths
- Consistent
getOpenCreateModal()method implementation- Proper type safety with generic return types
- Good separation of concerns
This consistency will make the test utilities easy to maintain and extend.
145-184: Well-designed library page with comprehensive tab interactions.The
LibraryListViewPageprovides excellent functionality:
- Clear locator initialization in constructor
- Intuitive method names for tab interactions
- Proper use of internationalization with
safeTranslate- Good separation between checking and getting library count
- Appropriate conditional logic for different tab states
The implementation follows testing best practices for page object models.
22-22: No impact from URLModel slash removal
After inspecting usage across the codebase and in our test helpers, allURLModelvalues are meant to represent model names (without any leading slash) and are concatenated with explicit/separators when building API endpoints. In tests, the browser‐route prefix remains'/folders'in theListViewPageconstructor, andURLModel: 'folders'is still used only as a context key (e.g. inModelTableRow). Removing the leading slash from theURLModelstring will not break routing or API calls.
| let optionCount = 0; | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| const classes = await searchBox.getAttribute('class'); | ||
| if (classes && classes.indexOf('disabled') >= 0) { | ||
| return 1; | ||
| } | ||
| optionCount = await this._self.locator('[role="option"]').count(); | ||
| return optionCount; // document.querySelectorAll(`[role="option"]`); | ||
| }, | ||
| { timeout: 20_000 } | ||
| ) | ||
| .toBeGreaterThan(0); | ||
|
|
||
| if (optionCount <= 1) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in polling logic.
The polling logic checks if the searchbox is disabled and returns 1, but then immediately checks optionCount > 0. This could create inconsistent behavior where a disabled searchbox passes the poll but then triggers an early return.
let optionCount = 0;
await expect
.poll(
async () => {
const classes = await searchBox.getAttribute('class');
if (classes && classes.indexOf('disabled') >= 0) {
- return 1;
+ return 0; // Return 0 for disabled state to fail the poll
}
optionCount = await this._self.locator('[role="option"]').count();
return optionCount;
},
{ timeout: 20_000 }
)
.toBeGreaterThan(0);🤖 Prompt for AI Agents
In frontend/tests/utilsv2/base/autocomplete-select-input.ts around lines 19 to
36, the polling logic returns 1 if the search box is disabled, which can cause
the subsequent check on optionCount to behave inconsistently. To fix this,
modify the polling function to return 0 or a distinct value when the search box
is disabled so that the poll does not falsely pass, and adjust the logic after
polling to handle the disabled state explicitly before checking optionCount.
| const start = Date.now(); | ||
| while (true) { | ||
| const html = await lastOption.innerHTML(); | ||
| if (html !== 'No matching options<!---->') break; | ||
| if (Date.now() - start > 10_000) { | ||
| expect(false, 'Expected condition was not met').toBeTruthy(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace busy wait loop with Playwright's built-in waiting mechanisms.
The manual while loop with Date.now() is an anti-pattern in Playwright. Use Playwright's waiting mechanisms for better reliability and performance.
- const start = Date.now();
- while (true) {
- const html = await lastOption.innerHTML();
- if (html !== 'No matching options<!---->') break;
- if (Date.now() - start > 10_000) {
- expect(false, 'Expected condition was not met').toBeTruthy();
- }
- }
+ await expect(lastOption).not.toHaveText('No matching options');📝 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.
| const start = Date.now(); | |
| while (true) { | |
| const html = await lastOption.innerHTML(); | |
| if (html !== 'No matching options<!---->') break; | |
| if (Date.now() - start > 10_000) { | |
| expect(false, 'Expected condition was not met').toBeTruthy(); | |
| } | |
| } | |
| - const start = Date.now(); | |
| - while (true) { | |
| - const html = await lastOption.innerHTML(); | |
| - if (html !== 'No matching options<!---->') break; | |
| - if (Date.now() - start > 10_000) { | |
| - expect(false, 'Expected condition was not met').toBeTruthy(); | |
| - } | |
| - } | |
| + await expect(lastOption).not.toHaveText('No matching options'); |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/base/autocomplete-select-input.ts around lines 41 to
48, replace the manual busy wait loop that uses Date.now() and a while(true)
construct with Playwright's built-in waiting methods such as waitForFunction or
waitForSelector. This will improve reliability and performance by letting
Playwright handle the waiting internally instead of a manual loop. Use an
appropriate Playwright wait method to wait until lastOption.innerHTML() is not
'No matching options<!---->' or until a timeout occurs.
| // This must become a protected method like _getOpenCreateModal | ||
| async getOpenDuplicateModal<T = CreateModal>(createModalClass: Element.Class<T>): Promise<T> { | ||
| // Should it be renamed "add-button-elem" instead ? | ||
| const locator = this._self.getByTestId('duplicate-button-elem'); | ||
| await locator.click(); | ||
|
|
||
| return this._getSubElement(createModalClass); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment and fix misleading comment.
The method implementation is correct and follows established patterns. However, there are two issues to address:
- The TODO comment suggests this should be a protected method like
_getOpenCreateModal - Line 38 comment incorrectly mentions "add-button-elem" when it should reference "duplicate-button-elem"
Consider making this method protected and fixing the comment:
- // This must become a protected method like _getOpenCreateModal
- async getOpenDuplicateModal<T = CreateModal>(createModalClass: Element.Class<T>): Promise<T> {
- // Should it be renamed "add-button-elem" instead ?
+ protected async _getOpenDuplicateModal<T = CreateModal>(createModalClass: Element.Class<T>): Promise<T> {
+ // Uses duplicate-button-elem test ID
const locator = this._self.getByTestId('duplicate-button-elem');
await locator.click();
return this._getSubElement(createModalClass);
}📝 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.
| // This must become a protected method like _getOpenCreateModal | |
| async getOpenDuplicateModal<T = CreateModal>(createModalClass: Element.Class<T>): Promise<T> { | |
| // Should it be renamed "add-button-elem" instead ? | |
| const locator = this._self.getByTestId('duplicate-button-elem'); | |
| await locator.click(); | |
| return this._getSubElement(createModalClass); | |
| } | |
| protected async _getOpenDuplicateModal<T = CreateModal>(createModalClass: Element.Class<T>): Promise<T> { | |
| // Uses duplicate-button-elem test ID | |
| const locator = this._self.getByTestId('duplicate-button-elem'); | |
| await locator.click(); | |
| return this._getSubElement(createModalClass); | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/base/detail-view-page.ts around lines 36 to 43, change
the method getOpenDuplicateModal from public to protected to match the pattern
of _getOpenCreateModal, and update the inline comment on line 38 to correctly
reference "duplicate-button-elem" instead of "add-button-elem" to avoid
confusion.
| /** Returns the number of currently loaded libraries. */ | ||
| async getLoadedLibraryCount(): Promise<number> { | ||
| const text = await this._loadedLibrariesTab.innerText(); | ||
| const libraryCount = Number(text.match(/[0-9]+$/)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of library count parsing.
The regex pattern /[0-9]+$/ might fail if the text format changes or contains no numbers, potentially returning NaN.
Add error handling for more robust parsing:
- const libraryCount = Number(text.match(/[0-9]+$/));
+ const match = text.match(/[0-9]+$/);
+ const libraryCount = match ? Number(match[0]) : 0;📝 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.
| const libraryCount = Number(text.match(/[0-9]+$/)); | |
| const match = text.match(/[0-9]+$/); | |
| const libraryCount = match ? Number(match[0]) : 0; |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/list-view.ts at line 181, the current regex
extraction of libraryCount using /[0-9]+$/ can return NaN if no match is found.
Modify the code to first check if the regex match result is not null before
converting to Number. If the match is null, assign a default value (e.g., 0) to
libraryCount to prevent NaN and improve robustness.
| // The AutoCompleteSelect should be later handled by a class instead of a single function like this. | ||
| async function selectChoice(expect: Expect, input: Locator, value: string) { | ||
| const inputSearch = input.locator('ul.selected input'); | ||
| const activeOption = input.locator('[role="option"].active'); | ||
| const searchBox = input.getByRole('searchbox'); | ||
|
|
||
| let optionCount = 0; | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| const classes = await searchBox.getAttribute('class'); | ||
| if (classes && classes.indexOf('disabled') >= 0) { | ||
| return 1; | ||
| } | ||
| optionCount = await input.locator('[role="option"]').count(); | ||
| return optionCount; | ||
| }, | ||
| { timeout: 20_000 } | ||
| ) | ||
| .toBeGreaterThan(0); | ||
|
|
||
| if (optionCount <= 1) { | ||
| return; | ||
| } | ||
|
|
||
| await inputSearch.fill(value); | ||
| const lastOption = input.locator('[role="option"]').last(); | ||
|
|
||
| const start = Date.now(); | ||
| while (true) { | ||
| const html = await lastOption.innerHTML(); | ||
| if (html !== 'No matching options<!---->') break; | ||
| if (Date.now() - start > 10_000) { | ||
| expect(false, 'Expected condition was not met').toBeTruthy(); | ||
| } | ||
| } | ||
| await lastOption.evaluate((node) => node.classList.add('active')); | ||
| await activeOption.click({ force: true }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the selectChoice helper function into a proper class method.
The comment on line 6 already acknowledges this should be handled by a class. The current implementation has several concerns:
- Complex polling logic with hardcoded timeouts
- Manual class manipulation with
evaluate() - Force clicking which can mask UI issues
- Multiple responsibilities mixed together
Move this logic into the AutoCompleteSelect class to improve encapsulation and reusability:
-// The AutoCompleteSelect should be later handled by a class instead of a single function like this.
-async function selectChoice(expect: Expect, input: Locator, value: string) {
- // ... complex implementation
-}The AutoCompleteSelect class should handle all its interaction logic internally.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/model-form.ts between lines 6 and 44, refactor
the standalone async function selectChoice into a method inside the
AutoCompleteSelect class. This involves moving all the polling, input filling,
option selection, and DOM manipulation logic into the class to encapsulate
behavior. Replace hardcoded timeouts with configurable or class-level settings,
avoid direct DOM class manipulation by using class methods or state, and remove
force clicking by implementing proper waiting and interaction flows within the
class. This will improve code organization, reusability, and maintainability.
| if (!choice) { | ||
| console.error( | ||
| `[ERROR:doApplyFilter] the '${value}' hasn't been found in the '${field}' filter.` | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider throwing error instead of console logging.
Console logging in tests doesn't fail the test when values are not found. Consider throwing an error or using expect assertions to make the test fail explicitly.
if (!choice) {
- console.error(
- `[ERROR:doApplyFilter] the '${value}' hasn't been found in the '${field}' filter.`
- );
- continue;
+ throw new Error(`Filter value '${value}' not found in field '${field}' filter`);
}📝 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.
| if (!choice) { | |
| console.error( | |
| `[ERROR:doApplyFilter] the '${value}' hasn't been found in the '${field}' filter.` | |
| ); | |
| continue; | |
| } | |
| if (!choice) { | |
| throw new Error(`Filter value '${value}' not found in field '${field}' filter`); | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/model-table-filter.ts around lines 47 to 52,
replace the console.error call with throwing an error or using an expect
assertion to explicitly fail the test when the 'choice' is not found. This
change will ensure the test fails immediately instead of just logging an error.
| // This class is dead code, but let's keep it for now. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider adding dead code to the codebase.
Adding code that's explicitly marked as "dead code" can lead to maintenance confusion and technical debt. Consider either:
- Removing this class if it's truly not needed
- Removing the dead code comment if the class serves a purpose
- Documenting the intended future use case if this is preparatory work
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/model-table-footer.ts around lines 5 to 6, the
class is marked as dead code. To address this, either remove the class entirely
if it is not needed, remove the dead code comment if the class is still
relevant, or add documentation explaining its intended future use to clarify its
purpose and avoid confusion.
| async gotoDetailView(expect: Expect): Promise<DetailViewPage> { | ||
| const detailButton = await this._self.getByTestId('tablerow-detail-button'); | ||
| const detailViewHref = detailButton.getAttribute('href'); | ||
| expect(detailViewHref).not.toBeNull(); | ||
| await this.getSelf().click(); |
There was a problem hiding this comment.
Fix async operation and null assertion issues.
Multiple issues in the detail view navigation:
- The
detailButton.getAttribute('href')call is not awaited - The expect assertion on
detailViewHrefwon't work as intended since it's a Promise - Potential null pointer if href attribute doesn't exist
async gotoDetailView(expect: Expect): Promise<DetailViewPage> {
- const detailButton = await this._self.getByTestId('tablerow-detail-button');
- const detailViewHref = detailButton.getAttribute('href');
- expect(detailViewHref).not.toBeNull();
+ const detailButton = this._self.getByTestId('tablerow-detail-button');
+ const detailViewHref = await detailButton.getAttribute('href');
+ await expect(detailButton).toHaveAttribute('href');
await this.getSelf().click();
- return this._goto(DetailViewPage, detailViewHref as string);
+ return this._goto(DetailViewPage, detailViewHref!);
}📝 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 gotoDetailView(expect: Expect): Promise<DetailViewPage> { | |
| const detailButton = await this._self.getByTestId('tablerow-detail-button'); | |
| const detailViewHref = detailButton.getAttribute('href'); | |
| expect(detailViewHref).not.toBeNull(); | |
| await this.getSelf().click(); | |
| async gotoDetailView(expect: Expect): Promise<DetailViewPage> { | |
| const detailButton = this._self.getByTestId('tablerow-detail-button'); | |
| const detailViewHref = await detailButton.getAttribute('href'); | |
| await expect(detailButton).toHaveAttribute('href'); | |
| await this.getSelf().click(); | |
| return this._goto(DetailViewPage, detailViewHref!); | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/model-table-row.ts around lines 31 to 35, fix
the async handling by awaiting the call to detailButton.getAttribute('href')
since it returns a Promise. Then, update the expect assertion to handle the
awaited value correctly, ensuring it is not null before proceeding. Add a null
check or assertion to prevent potential null pointer errors if the href
attribute is missing. This will ensure proper async operation and safe null
handling.
| async doDeleteObject(): Promise<void> { | ||
| await this._deleteButton.click(); | ||
|
|
||
| if (this._getContext('URLModel') === 'folders') { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic string to constant.
The hardcoded 'folders' string should be extracted to a constant for maintainability.
+ private static readonly FOLDER_URL_MODEL = 'folders';
+
async doDeleteObject(): Promise<void> {
await this._deleteButton.click();
- if (this._getContext('URLModel') === 'folders') {
+ if (this._getContext('URLModel') === ModelTableRow.FOLDER_URL_MODEL) {📝 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.
| if (this._getContext('URLModel') === 'folders') { | |
| private static readonly FOLDER_URL_MODEL = 'folders'; | |
| async doDeleteObject(): Promise<void> { | |
| await this._deleteButton.click(); | |
| if (this._getContext('URLModel') === ModelTableRow.FOLDER_URL_MODEL) { |
🤖 Prompt for AI Agents
In frontend/tests/utilsv2/derived/model-table-row.ts at line 61, the string
'folders' is used directly as a magic string. Extract this string into a named
constant at the top of the file or in an appropriate constants file, then
replace the hardcoded 'folders' in the _getContext call with this constant to
improve maintainability.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores