[TypeScript] Convert test page models to TS #4218
Conversation
actual-github-bot
bot
changed the title
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
joel-jeremy
force-pushed
the
ts-playwright-page-models
branch
from
e7935dc to
12bfde8
Compare
|
I'm wondering why these files are in kebab-case instead of camelCase. I think we should rename these to camelCase to match the convention in other parts of the codebase. |
joel-jeremy
force-pushed
the
ts-playwright-page-models
branch
from
ce6023d to
7028424
Compare
joel-jeremy
force-pushed
the
ts-playwright-tests
branch
from
d6c78e4 to
43f73cf
Compare
joel-jeremy
force-pushed
the
ts-playwright-page-models
branch
2 times, most recently
from
a34a0ab to
ce4451d
Compare
joel-jeremy
force-pushed
the
ts-playwright-page-models
branch
from
ce4451d to
8f8ab77
Compare
joel-jeremy
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe pull request introduces comprehensive TypeScript type annotations and refactoring across multiple page model files in the desktop client's end-to-end testing suite. The changes primarily focus on enhancing type safety, improving error handling, and standardizing class structures across various page models. Key modifications include explicitly typing constructor parameters, adding readonly properties, introducing specific type definitions for method parameters, and refining error handling mechanisms. The changes span multiple files in the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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
🧹 Nitpick comments (34)
packages/desktop-client/e2e/page-models/navigation.ts (3)
Line range hint
22-67: Consider adding error handling for navigation failures.While the navigation methods are well-typed, consider adding try-catch blocks to handle potential navigation failures gracefully. This would improve test reliability by providing clear error messages when elements are not found or clicks fail.
Example improvement:
async goToAccountPage(accountName: string) { + try { await this.page .getByRole('link', { name: new RegExp(`^${accountName}`) }) .click(); return new AccountPage(this.page); + } catch (error) { + throw new Error(`Failed to navigate to account "${accountName}": ${error.message}`); + } }
Line range hint
68-85: Consider adding input validation for the account creation form.While the method correctly uses the
AccountEntrytype, consider adding validation for the input data before filling the form. For example, validate that the balance is non-negative and the account name is not empty.Example improvement:
async createAccount(data: AccountEntry) { + if (!data.name.trim()) { + throw new Error('Account name cannot be empty'); + } + if (data.balance < 0) { + throw new Error('Account balance cannot be negative'); + } await this.page.getByRole('button', { name: 'Add account' }).click(); // ... rest of the method }
2-8: Consider standardizing file naming convention.As mentioned in the PR comments, there's an inconsistency in the file naming convention (kebab-case vs camelCase). Consider renaming the imported files to follow the camelCase convention used in other parts of the codebase:
account-page.ts→accountPage.tsreports-page.ts→reportsPage.tsrules-page.ts→rulesPage.tsschedules-page.ts→schedulesPage.tssettings-page.ts→settingsPage.tspackages/desktop-client/e2e/page-models/configuration-page.ts (2)
8-8: Consider enhancing type safety for the heading locator.The heading locator could be more type-safe by specifying the role type in the
getByRolecall.- readonly heading: Locator; + readonly heading: Locator & { heading: true }; - this.heading = page.getByRole('heading'); + this.heading = page.getByRole('heading') as Locator & { heading: true };Also applies to: 13-13
Line range hint
31-68: Consider refactoring the switch statement to reduce duplication.The switch statement contains repeated patterns that could be extracted into a configuration object or helper method.
+ private readonly importConfig = { + YNAB4: { + buttonName: 'YNAB4 The old unsupported desktop app', + fileButtonName: 'Select zip file...', + }, + nYNAB: { + buttonName: 'nYNAB The newer web app', + fileButtonName: 'Select file...', + }, + Actual: { + buttonName: 'Actual Import a file exported from Actual', + fileButtonName: 'Select file...', + }, + } as const; + async importBudget(type: 'YNAB4' | 'nYNAB' | 'Actual', file: string) { const fileChooserPromise = this.page.waitForEvent('filechooser'); await this.page.getByRole('button', { name: 'Import my budget' }).click(); - switch (type) { - case 'YNAB4': - await this.page - .getByRole('button', { - name: 'YNAB4 The old unsupported desktop app', - }) - .click(); - await this.page - .getByRole('button', { name: 'Select zip file...' }) - .click(); - break; - - case 'nYNAB': - await this.page - .getByRole('button', { name: 'nYNAB The newer web app' }) - .click(); - await this.page.getByRole('button', { name: 'Select file...' }).click(); - break; - - case 'Actual': - await this.page - .getByRole('button', { - name: 'Actual Import a file exported from Actual', - }) - .click(); - await this.page.getByRole('button', { name: 'Select file...' }).click(); - break; - - default: - throw new Error(`Unrecognized import type: ${type}`); + const config = this.importConfig[type]; + if (!config) { + throw new Error(`Unrecognized import type: ${type}`); } + await this.page + .getByRole('button', { name: config.buttonName }) + .click(); + await this.page + .getByRole('button', { name: config.fileButtonName }) + .click(); + const fileChooser = await fileChooserPromise; await fileChooser.setFiles(file); return new BudgetPage(this.page); }packages/desktop-client/e2e/page-models/mobile-navigation.ts (1)
99-102: Consider creating an interface for page models.The generic type constraint
{ waitFor: Locator['waitFor'] }could be more maintainable as a proper interface.+interface PageModel { + waitFor: Locator['waitFor']; +} + async navigateToPage<T extends { waitFor: Locator['waitFor'] }>( - pageName: keyof typeof ROUTES_BY_PAGE, - pageModelFactory: () => T, + pageName: keyof typeof ROUTES_BY_PAGE, + pageModelFactory: () => T extends PageModel ? T : never, ): Promise<T>This would:
- Make the code more maintainable
- Allow for adding more required methods to page models in the future
- Improve type safety
packages/desktop-client/e2e/page-models/rules-page.ts (5)
21-26: Consider refining theRuleEntrytype definition.A few suggestions to improve the type definition:
- The
conditionsOpfield acceptsRegExp, which seems unnecessary for UI testing as the UI likely only accepts string values.- The
splitstype could be simplified fromArray<SplitsEntry[]>toSplitsEntry[][]for better readability.type RuleEntry = { - conditionsOp?: string | RegExp; + conditionsOp?: string; conditions?: ConditionsEntry[]; actions?: ActionsEntry[]; - splits?: Array<SplitsEntry[]>; + splits?: SplitsEntry[][]; };
3-26: Add JSDoc comments to type definitions.Consider adding JSDoc comments to explain the purpose and usage of each type definition. This would improve code documentation and help other developers understand how to use these types correctly.
/** Represents a condition in a rule with field, operator, and value. */ type ConditionsEntry = { field: string; op: string; value: string; }; /** Represents an action in a rule with optional operator. */ type ActionsEntry = { field: string; op?: string; value: string; };
Line range hint
56-63: Add return type annotation togetNthRule.The method's return type should be explicitly defined for better type safety and documentation.
- getNthRule(index: number) { + getNthRule(index: number): { conditions: Locator; actions: Locator } {
40-40: Enhance method documentation.The JSDoc comments could be more descriptive:
createRule: Add parameter description and example usagegetNthRule: Clarify that index is 0-basedsearchFor: Add JSDoc comment/** * Creates a new rule with the specified configuration. * @param data - The rule configuration containing conditions, actions, and splits * @example * await createRule({ * conditions: [{ field: 'amount', op: '>', value: '100' }], * actions: [{ field: 'category', value: 'Food' }] * }); */Also applies to: 56-56, 65-65
Line range hint
69-108: Refactor_fillRuleFieldsfor better maintainability.The method has complex nested logic that could be simplified:
- Extract the split actions logic into a separate method
- Remove the magic number calculation for split index
async _fillRuleFields(data: RuleEntry) { if (data.conditionsOp) { await this._fillConditionsOp(data.conditionsOp); } if (data.conditions) { await this._fillEditorFields( data.conditions, this.page.getByTestId('condition-list'), true, ); } if (data.actions) { await this._fillEditorFields( data.actions, this.page.getByTestId('action-list'), ); } if (data.splits) { - let idx = data.actions?.length ?? 0; + const startIndex = data.actions?.length ?? 0; await this._fillSplitActions(data.splits, startIndex); } } +private async _fillSplitActions(splits: SplitsEntry[][], startIndex: number) { + for (const [idx, splitActions] of splits.entries()) { + await this.page.getByTestId('add-split-transactions').click(); + await this._fillEditorFields( + splitActions, + this.page.getByTestId('action-list').nth(startIndex + idx), + ); + } +}packages/desktop-client/e2e/page-models/budget-page.ts (1)
19-53: Refactor similar methods to eliminate duplicationThe methods
getTotalBudgeted,getTotalSpent, andgetTotalLeftoverhave similar implementations. Consider refactoring them into a single helper method to adhere to the DRY principle and improve maintainability.Implement a helper method:
private async getTotalAmountByTestId(testIdPattern: RegExp, errorMessage: string): Promise<number> { const totalText = await this.budgetTableTotals .getByTestId(testIdPattern) .textContent(); if (!totalText) { throw new Error(errorMessage); } return parseInt(totalText, 10); }Then refactor the methods:
async getTotalBudgeted(): Promise<number> { return this.getTotalAmountByTestId(/total-budgeted$/, 'Failed to get total budgeted.'); } async getTotalSpent(): Promise<number> { return this.getTotalAmountByTestId(/total-spent$/, 'Failed to get total spent.'); } async getTotalLeftover(): Promise<number> { return this.getTotalAmountByTestId(/total-leftover$/, 'Failed to get total leftover.'); }packages/desktop-client/e2e/page-models/close-account-modal.ts (1)
12-15: Consider using role-based selector instead of keyboard Enter.The current implementation using
keyboard.press('Enter')after fill is fragile. Based on previous learnings from PR #3499, consider using role-based selectors with aria-labels for more robust test automation.async selectTransferAccount(accountName: string) { await this.locator.getByPlaceholder('Select account...').fill(accountName); - await this.page.keyboard.press('Enter'); + await this.locator + .getByRole('option', { name: accountName }) + .click(); }packages/desktop-client/e2e/page-models/mobile-category-menu-modal.ts (2)
Line range hint
17-18: Consider using role-based selector instead of test ID.Replace
getByTestId('amount-input')with a more semantic role-based selector for better accessibility and maintainability.- this.budgetAmountInput = locator.getByTestId('amount-input'); + this.budgetAmountInput = locator.getByRole('spinbutton', { name: 'Budget amount' });
27-29: Consider caching the EditNotesModal instance.The current implementation creates a new EditNotesModal instance on every call. Consider caching it as a readonly property for better performance.
+ private editNotesModal: EditNotesModal | null = null; + async editNotes() { await this.editNotesButton.click(); - return new EditNotesModal(this.page.getByRole('dialog')); + if (!this.editNotesModal) { + this.editNotesModal = new EditNotesModal(this.page.getByRole('dialog')); + } + return this.editNotesModal; }packages/desktop-client/e2e/page-models/mobile-accounts-page.ts (1)
Line range hint
1-41: Consider renaming file to follow camelCase convention.Based on the PR discussion, consider renaming this file from
mobile-accounts-page.tstomobileAccountsPage.tsto maintain consistency with the camelCase convention used in other parts of the codebase.Well-structured TypeScript implementation!
Great job on the TypeScript conversion:
- Proper use of readonly properties
- Good type safety with Parameters utility type
- Clear method documentation
- Consistent use of Playwright's Page and Locator types
packages/desktop-client/e2e/page-models/reports-page.ts (1)
Line range hint
1-45: Consider renaming file and improving return types.
- Consider renaming from
reports-page.tstoreportsPage.tsto follow camelCase convention.- Navigation methods could benefit from explicit return types:
- async goToNetWorthPage() { + async goToNetWorthPage(): Promise<ReportsPage> { await this.pageContent.getByRole('button', { name: /^Net/ }).click(); return new ReportsPage(this.page); } - async goToCashFlowPage() { + async goToCashFlowPage(): Promise<ReportsPage> { await this.pageContent.getByRole('button', { name: /^Cash/ }).click(); return new ReportsPage(this.page); } - async goToCustomReportPage() { + async goToCustomReportPage(): Promise<CustomReportPage> { await this.pageContent .getByRole('button', { name: 'Add new widget' }) .click(); await this.page.getByRole('button', { name: 'New custom report' }).click(); return new CustomReportPage(this.page); }Well-structured page object implementation!
Great job on:
- Proper use of readonly properties
- Consistent pattern for navigation methods
- Clear separation of concerns
packages/desktop-client/e2e/page-models/custom-report-page.ts (1)
Line range hint
1-41: Consider renaming file to follow camelCase convention.Consider renaming from
custom-report-page.tstocustomReportPage.tsto maintain consistency with camelCase convention.Excellent TypeScript implementation!
Great job on:
- Strong typing with string literal union for mode parameter
- Proper exhaustive check in switch statement
- Well-organized readonly properties
- Clear button locator grouping
packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.ts (2)
Line range hint
1-35: Consider renaming file and adding return type.
- Consider renaming from
mobile-balance-menu-modal.tstomobileBalanceMenuModal.tsto follow camelCase convention.- Add return type to close method:
- async close() { + async close(): Promise<void> { await this.heading.getByRole('button', { name: 'Close' }).click(); }Well-structured modal implementation!
Great job on:
- Efficient constructor using locator.page()
- Proper use of readonly properties
- Clear organization of button locators
1-1: Summary of TypeScript conversion reviewThe TypeScript conversion is well-implemented across all files. Consider these improvements:
- Rename all files to follow camelCase convention for consistency:
mobile-accounts-page.ts→mobileAccountsPage.tsreports-page.ts→reportsPage.tscustom-report-page.ts→customReportPage.tsmobile-balance-menu-modal.ts→mobileBalanceMenuModal.ts- Add explicit return types to async methods where missing.
Overall, great job on:
- Proper use of TypeScript features
- Consistent page object model pattern
- Well-organized readonly properties
- Clear separation of concerns
packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts (1)
1-1: Consider renaming file to follow TypeScript conventions.The file uses kebab-case naming which differs from TypeScript's recommended camelCase convention. Consider renaming to
mobileTransactionEntryPage.tsfor consistency with TypeScript standards.packages/desktop-client/e2e/page-models/mobile-account-page.ts (3)
1-1: Consider renaming file to follow TypeScript conventions.Similar to other files, consider renaming to
mobileAccountPage.tsto follow TypeScript naming conventions.
37-41: Enhance error handling with specific error message.Consider providing more context in the error message about why the balance couldn't be retrieved.
- throw new Error('Failed to get balance.'); + throw new Error('Failed to get balance: balance element returned empty text content.');
44-48: Enhance JSDoc comments with type information.Consider adding @param and @returns annotations to improve documentation.
/** * Search by the given term + * @param term - The search term to filter transactions + * @returns Promise<void> */packages/desktop-client/e2e/page-models/settings-page.ts (3)
1-1: Consider renaming file to follow TypeScript conventions.Consider renaming to
settingsPage.tsto follow TypeScript naming conventions.
Line range hint
17-20: Document the regex pattern used in the locator.Add a comment explaining the regex pattern's purpose and expected matches.
+ // Matches either "Switch to envelope budgeting" or "Switch to tracking budgeting" this.switchBudgetTypeButton = this.settings.getByRole('button', { name: /^Switch to (envelope|tracking) budgeting$/i, });
Line range hint
44-57: Consider refactoring to reduce nesting.The enableExperimentalFeature method could be simplified to reduce nesting and improve readability.
async enableExperimentalFeature(featureName: string) { - if (await this.advancedSettingsButton.isVisible()) { - await this.advancedSettingsButton.click(); - } - - if (await this.experimentalSettingsButton.isVisible()) { - await this.experimentalSettingsButton.click(); - } + const buttons = [this.advancedSettingsButton, this.experimentalSettingsButton]; + for (const button of buttons) { + if (await button.isVisible()) { + await button.click(); + } + } const featureCheckbox = this.page.getByRole('checkbox', { name: featureName, }); if (!(await featureCheckbox.isChecked())) { await featureCheckbox.click(); } }packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.ts (3)
1-1: Consider renaming file to follow TypeScript conventions.Consider renaming to
mobileBudgetMenuModal.tsto follow TypeScript naming conventions.
8-12: Consider refactoring similar button locators.Multiple button locators follow a similar pattern. Consider creating a helper method to reduce code duplication.
+ private getActionButton(name: string): Locator { + return this.locator.getByRole('button', { name }); + } + constructor(locator: Locator) { this.locator = locator; this.page = locator.page(); this.heading = locator.getByRole('heading'); this.budgetAmountInput = locator.getByTestId('amount-input'); - this.copyLastMonthBudgetButton = locator.getByRole('button', { - name: 'Copy last month's budget', - }); + this.copyLastMonthBudgetButton = this.getActionButton('Copy last month's budget'); // Apply similar changes to other button locators }
41-44: Add error handling for setBudgetAmount.Consider adding validation and error handling for the budget amount input.
async setBudgetAmount(newAmount: string) { + if (!newAmount.match(/^\d*\.?\d*$/)) { + throw new Error('Invalid budget amount format. Expected a numeric value.'); + } await this.budgetAmountInput.fill(newAmount); await this.budgetAmountInput.blur(); await this.close(); }packages/desktop-client/e2e/page-models/schedules-page.ts (1)
Line range hint
59-76: Consider adding explicit return types to methods.While TypeScript can infer return types, explicit return types improve code clarity and serve as documentation. For example:
- async _performNthAction(index: number, actionName: string | RegExp) { + async _performNthAction(index: number, actionName: string | RegExp): Promise<void> {packages/desktop-client/e2e/page-models/mobile-budget-page.ts (1)
Line range hint
300-324: Consider consolidating similar error handling patterns.The error handling for budget summary buttons follows a similar pattern. Consider extracting the common logic into a shared utility function.
private async getButtonWithFallback<T extends Locator>( buttons: T[], errorMessage: string, throwIfNotFound = true ): Promise<T | null> { for (const button of buttons) { if (await button.isVisible()) { return button; } } if (!throwIfNotFound) { return null; } throw new Error(errorMessage); }Also applies to: 330-358
packages/desktop-client/e2e/budget.mobile.test.ts (2)
63-65: LGTM! Consider adding more context to the error message.The error handling is a good addition to prevent silent failures.
Consider making the error message more descriptive:
- throw new Error('Failed to get spent amount'); + throw new Error(`Failed to get spent amount for category "${categoryName}"`);
286-289: LGTM! Consider adding more context to the error message.The error handling is a good addition to prevent silent failures.
Consider making the error message more descriptive:
- throw new Error('Failed to get last month budget'); + throw new Error(`Failed to get last month budget for category "${categoryName}"`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4218.mdis excluded by!**/*.md
📒 Files selected for processing (26)
packages/desktop-client/e2e/budget.mobile.test.ts(2 hunks)packages/desktop-client/e2e/page-models/account-page.ts(11 hunks)packages/desktop-client/e2e/page-models/budget-page.js(0 hunks)packages/desktop-client/e2e/page-models/budget-page.ts(1 hunks)packages/desktop-client/e2e/page-models/close-account-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/configuration-page.ts(2 hunks)packages/desktop-client/e2e/page-models/custom-report-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-account-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-accounts-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-budget-page.ts(11 hunks)packages/desktop-client/e2e/page-models/mobile-category-menu-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-navigation.ts(3 hunks)packages/desktop-client/e2e/page-models/mobile-reports-page.js(0 hunks)packages/desktop-client/e2e/page-models/mobile-reports-page.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts(2 hunks)packages/desktop-client/e2e/page-models/navigation.ts(2 hunks)packages/desktop-client/e2e/page-models/reports-page.ts(1 hunks)packages/desktop-client/e2e/page-models/rules-page.ts(4 hunks)packages/desktop-client/e2e/page-models/schedules-page.ts(5 hunks)packages/desktop-client/e2e/page-models/settings-page.ts(2 hunks)packages/desktop-client/e2e/transactions.test.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/e2e/page-models/budget-page.js
- packages/desktop-client/e2e/page-models/mobile-reports-page.js
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/e2e/page-models/close-account-modal.ts (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3499
File: packages/desktop-client/src/components/modals/CloseAccountModal.tsx:233-233
Timestamp: 2024-11-10T16:45:31.225Z
Learning: When Playwright cannot match a React link by text, adding an `aria-label` is acceptable to facilitate element selection in tests.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (24)
packages/desktop-client/e2e/page-models/navigation.ts (2)
1-13: LGTM! Well-structured type definitions.The
AccountEntrytype is well-defined with clear property types, and thePagetype is correctly imported using thetypeimport syntax.
16-20: LGTM! Good use of TypeScript features.The
readonlymodifier for the page property ensures immutability, and the constructor parameter is properly typed.packages/desktop-client/e2e/page-models/configuration-page.ts (2)
1-1: LGTM! Well-structured type imports.Good use of type-only imports for Playwright types, following TypeScript best practices.
7-10: LGTM! Well-structured class properties and constructor.Good use of readonly modifiers and explicit type annotations for class properties and constructor parameter.
packages/desktop-client/e2e/page-models/mobile-navigation.ts (3)
27-41: Well-structured class properties with proper TypeScript patterns!Good use of:
- readonly modifiers for immutable properties
- explicit type annotations
- clean property initialization in the constructor
46-48: Robust error handling with clear error messages!Good improvements in error handling:
- Thorough null checks for bounding boxes
- Clear and descriptive error messages
- Consistent error handling pattern across methods
Also applies to: 54-56, 72-74, 93-95
21-21: Verify if '/transactions/new' is the correct route for Transactions.The route '/transactions/new' seems to specifically point to a new transaction form rather than a general transactions list. Please verify if this is the intended behavior.
✅ Verification successful
The '/transactions/new' route is correctly implemented
The route is consistent with the mobile app's implementation, where the Transactions tab in mobile navigation intentionally directs users to the new transaction form, as evidenced by its usage in
MobileNavTabs.tsxandAddTransactionButton.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for route definitions and usages rg -A 5 "'/transactions/new'" packages/desktop-clientLength of output: 1713
packages/desktop-client/e2e/page-models/rules-page.ts (2)
29-34: LGTM! Properties and constructor are well-typed.The readonly properties and constructor implementation look good. The use of readonly prevents accidental modifications to these properties.
Line range hint
109-146: Add wait for UI interactions in_fillEditorFields.The method performs multiple UI interactions that could race. Consider adding appropriate waits:
async _fillEditorFields( data: Array<ConditionsEntry | ActionsEntry | SplitsEntry>, rootElement: Locator, fieldFirst = false, ) { for (const [idx, entry] of data.entries()) { const { field, op, value } = entry; const row = rootElement.getByTestId('editor-row').nth(idx); if (!(await row.isVisible())) { await rootElement.getByRole('button', { name: 'Add entry' }).click(); + await row.waitFor({ state: 'visible' }); } if (op && !fieldFirst) { await row.getByTestId('op-select').getByRole('button').first().click(); + await this.page.waitForSelector('role=button[name="${op}"]'); await this.page.getByRole('button', { name: op, exact: true }).click(); }Let's verify the UI interaction patterns in other test files:
✅ Verification successful
Use
waitFor()instead ofwaitForSelector()for UI interactionsThe need for waits is valid, but the implementation should follow the codebase's established patterns. Use
waitFor()on locators instead:async _fillEditorFields( data: Array<ConditionsEntry | ActionsEntry | SplitsEntry>, rootElement: Locator, fieldFirst = false, ) { for (const [idx, entry] of data.entries()) { const { field, op, value } = entry; const row = rootElement.getByTestId('editor-row').nth(idx); if (!(await row.isVisible())) { await rootElement.getByRole('button', { name: 'Add entry' }).click(); + await row.waitFor({ state: 'visible' }); } if (op && !fieldFirst) { await row.getByTestId('op-select').getByRole('button').first().click(); + await this.page.getByRole('button', { name: op, exact: true }).waitFor(); await this.page.getByRole('button', { name: op, exact: true }).click(); }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar UI interaction patterns to ensure consistency rg -U "waitFor|waitForSelector" "packages/desktop-client/e2e/page-models/"Length of output: 3139
packages/desktop-client/e2e/page-models/mobile-reports-page.ts (1)
3-16: Well-structured class implementationThe
MobileReportsPageclass is well-implemented with clear initialization and method definitions.packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.ts (1)
Line range hint
1-16: Code conversion to TypeScript is correctThe
EnvelopeBudgetSummaryModalclass has been correctly updated with TypeScript type annotations and properties. The constructor and method implementations are appropriate.packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.ts (1)
Line range hint
1-14: Well-structured TypeScript implementation!Good use of:
- Type-safe readonly properties
- Role-based selectors for accessibility
- Single responsibility principle in constructor
packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.ts (1)
Line range hint
4-16: Excellent TypeScript implementation with clear property initialization!Good practices:
- Consistent use of role-based selectors
- Clear readonly property declarations
- Well-organized constructor initialization
packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts (2)
6-11: Well-structured TypeScript implementation!Good use of readonly properties and proper typing of Playwright's Page and Locator types. This prevents accidental modifications and provides better type safety.
24-25: Excellent use of TypeScript utility types!The
Parameters<Locator['waitFor']>type effectively captures the parameter types of Playwright's waitFor method, ensuring type safety while maintaining flexibility.packages/desktop-client/e2e/page-models/schedules-page.ts (3)
Line range hint
1-29: Well-structured type definitions and class properties!The TypeScript conversion follows best practices:
- Proper type imports
- Well-defined ScheduleEntry interface with optional fields
- Readonly class properties for immutability
Line range hint
30-38: Clean constructor implementation and method typing!The TypeScript conversion maintains good practices:
- Properly typed constructor parameter
- Clear initialization of class properties
- Strong typing for the addNewSchedule method
Line range hint
84-103: Address the TODO comment regarding input field behavior.The comment indicates a known issue with input field change events during tests:
// For some readon, the input field does not trigger the change event on tests // but it works on the browser. We can revisit this once migration to // react aria components is complete.Would you like me to help investigate alternative approaches for handling input field events in tests, or create an issue to track this TODO?
✅ Verification successful
React Aria migration is already in progress - TODO comment is valid
The codebase shows active adoption of React Aria components across multiple features. The workaround using
keyboard.press('Enter')is consistently used in e2e tests, and the planned migration to React Aria components is the correct approach to resolve this input field behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check for React Aria related imports and components echo "=== Checking React Aria usage ===" rg --type ts --type tsx "from '@react-aria|from 'react-aria" -l echo -e "\n=== Looking for similar input field event handling patterns ===" rg --type ts --type tsx "keyboard\.press\('Enter'\)" -B 2 -A 2 echo -e "\n=== Checking for related TODOs about input fields ===" rg --type ts --type tsx "TODO.*input.*field|TODO.*change.*event" -B 1 -A 1 echo -e "\n=== Looking for React Aria migration mentions ===" rg --type ts --type tsx --type md "migration.*react.?aria|react.?aria.*migration" -iLength of output: 876
Script:
#!/bin/bash # Check for React Aria related imports and components echo "=== Checking React Aria usage ===" rg "from ['\"]([@]?)react-aria" echo -e "\n=== Looking for similar input field event handling patterns ===" rg "keyboard\.press\(['\"]Enter['\"]" -B 2 -A 2 echo -e "\n=== Checking for related TODOs about input fields ===" rg "TODO.*input.*field|TODO.*change.*event" -B 1 -A 1 echo -e "\n=== Looking for React Aria migration mentions ===" rg "migration.*react.?aria|react.?aria.*migration" -iLength of output: 6263
packages/desktop-client/e2e/transactions.test.ts (1)
43-43: Clean migration to using filterTooltip.locator!The changes correctly update the filterTooltip references to use the locator property, maintaining consistency with the TypeScript conversion.
Also applies to: 61-61
packages/desktop-client/e2e/page-models/account-page.ts (3)
5-12: Well-defined TransactionEntry type!The type definition is comprehensive and properly uses optional fields for flexibility.
15-29: Excellent use of readonly properties!The class properties are well-organized and properly typed with readonly modifiers for immutability.
Line range hint
1-361: Consider standardizing file naming convention.The file uses kebab-case (
account-page.ts) while some parts of the codebase use camelCase. This was noted in the PR comments.Let's check the prevalent naming convention in the codebase:
packages/desktop-client/e2e/page-models/mobile-budget-page.ts (2)
11-32: Excellent class property organization!The readonly properties are well-structured and properly typed, with clear naming that reflects their purpose.
97-102: Strong error handling implementation!The error handling is thorough and provides clear error messages. Good use of TypeScript's type system to ensure type safety in error scenarios.
Also applies to: 128-136, 164-169, 235-247
| const categoryNameText = this.budgetTable | ||
| .getByTestId('row') | ||
| .nth(idx) | ||
| .getByTestId('category-name') | ||
| .textContent(); |
There was a problem hiding this comment.
Fix missing await in getCategoryNameForRow method
In the getCategoryNameForRow method, the textContent() call returns a Promise, but there is no await before it. This means categoryNameText will be a Promise instead of a string, leading to incorrect behavior.
Apply this diff to fix the issue:
async getCategoryNameForRow(idx: number) {
- const categoryNameText = this.budgetTable
+ const categoryNameText = await this.budgetTable
.getByTestId('row')
.nth(idx)
.getByTestId('category-name')
.textContent();📝 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 categoryNameText = this.budgetTable | |
| .getByTestId('row') | |
| .nth(idx) | |
| .getByTestId('category-name') | |
| .textContent(); | |
| const categoryNameText = await this.budgetTable | |
| .getByTestId('row') | |
| .nth(idx) | |
| .getByTestId('category-name') | |
| .textContent(); |
| async getTableTotals() { | ||
| return { | ||
| budgeted: this.getTotalBudgeted(), | ||
| spent: this.getTotalSpent(), | ||
| balance: this.getTotalLeftover(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Await promises in getTableTotals method
The getTableTotals method calls asynchronous functions without await, resulting in properties containing Promise objects instead of the resolved values.
Apply this diff to fix the issue:
async getTableTotals() {
- return {
- budgeted: this.getTotalBudgeted(),
- spent: this.getTotalSpent(),
- balance: this.getTotalLeftover(),
- };
+ const [budgeted, spent, balance] = await Promise.all([
+ this.getTotalBudgeted(),
+ this.getTotalSpent(),
+ this.getTotalLeftover(),
+ ]);
+ return { budgeted, spent, balance };
}📝 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 getTableTotals() { | |
| return { | |
| budgeted: this.getTotalBudgeted(), | |
| spent: this.getTotalSpent(), | |
| balance: this.getTotalLeftover(), | |
| }; | |
| } | |
| async getTableTotals() { | |
| const [budgeted, spent, balance] = await Promise.all([ | |
| this.getTotalBudgeted(), | |
| this.getTotalSpent(), | |
| this.getTotalLeftover(), | |
| ]); | |
| return { budgeted, spent, balance }; | |
| } |
|
/update-vrt |
joel-jeremy
force-pushed
the
ts-playwright-page-models
branch
from
8f8ab77 to
b24aa95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.ts (2)
13-15: Consider adding constructor parameter validation.The constructor could benefit from validation to ensure the locator is properly initialized.
constructor(locator: Locator) { + if (!locator) { + throw new Error('Locator parameter is required'); + } this.locator = locator; this.page = locator.page();
Line range hint
1-32: Consider renaming file to follow camelCase convention.Based on the PR discussion about naming conventions, this file should be renamed from
mobile-balance-menu-modal.tstomobileBalanceMenuModal.tsto maintain consistency with the rest of the codebase.packages/desktop-client/e2e/page-models/mobile-budget-page.ts (5)
11-31: Consider grouping properties with JSDoc comments.While the properties are well-typed and properly marked as readonly, consider adding JSDoc comments to group them by functionality (e.g., navigation controls, summary buttons, table elements) for better code organization.
Example grouping:
/** Date format constant */ readonly MONTH_HEADER_DATE_FORMAT = 'MMMM 'yy'; /** Page reference */ readonly page: Page; /** Navigation controls */ readonly heading: Locator; readonly previousMonthButton: Locator; readonly selectedBudgetMonthButton: Locator; readonly nextMonthButton: Locator; /** Budget summary buttons */ readonly toBudgetButton: Locator; readonly overbudgetedButton: Locator; // ... other summary buttons /** Table elements */ readonly budgetTable: Locator; readonly categoryRows: Locator; // ... other table elements
Line range hint
33-96: Consider simplifying long locator chains.The locator chains for category-related elements could be simplified by storing intermediate locators as private readonly properties. This would improve readability and reusability.
Example refactor:
class MobileBudgetPage { // Add these private properties readonly #budgetGroups: Locator; constructor(page: Page) { // ... other initializations ... this.#budgetGroups = this.budgetTable.getByTestId('budget-groups'); // Simplify these chains this.categoryRows = this.#budgetGroups.getByTestId('category-row'); this.categoryNames = this.categoryRows.getByTestId('category-name'); this.categoryGroupRows = this.#budgetGroups.getByTestId('category-group-row'); this.categoryGroupNames = this.categoryGroupRows.getByTestId('category-group-name'); } }
Line range hint
109-124: Extract retry configuration and timeouts to class constants.The
maxAttemptsand timeout values should be defined as class constants for better maintainability and consistency.class MobileBudgetPage { private static readonly MAX_ATTEMPTS = 3; private static readonly RETRY_TIMEOUT = 1000; async toggleVisibleColumns({ maxAttempts = MobileBudgetPage.MAX_ATTEMPTS, }: { maxAttempts?: number } = {}) { // ... same implementation with MobileBudgetPage.RETRY_TIMEOUT } }
Line range hint
249-269: Consider implementing a generic retry utility.The retry logic in
#waitForNewMonthToLoadis similar to other methods. Consider extracting it into a reusable utility function.private async retry<T>({ action, condition, errorMessage, maxAttempts = MobileBudgetPage.MAX_ATTEMPTS, timeout = MobileBudgetPage.RETRY_TIMEOUT }: { action: () => Promise<T>, condition: (result: T) => boolean, errorMessage: string, maxAttempts?: number, timeout?: number }): Promise<T> { for (let attempt = 0; attempt < maxAttempts; attempt++) { const result = await action(); if (condition(result)) { return result; } await this.page.waitForTimeout(timeout); } throw new Error(errorMessage); } // Usage in #waitForNewMonthToLoad return await this.retry({ action: () => this.getSelectedMonth(), condition: (month) => month !== currentMonth, errorMessage: 'Failed to navigate to the new month.', });
Line range hint
186-212: Enhance error messages with more context.The error messages in
#getButtonForCellcould include more context about the current state of the page.throw new Error( `${buttonType} button for category "${categoryName}" could not be located. ` + `Current visible columns: ${await this.getVisibleColumns()}. ` + `Please ensure the category exists and the column is visible.` );packages/desktop-client/e2e/page-models/schedules-page.ts (1)
Line range hint
84-90: Typo in comment: 'readon' should be 'reason'There's a typo in the comment on line 86: "For some readon" should be "For some reason".
Apply this diff to correct the typo:
// For some readon, the input field does not trigger the change event on tests + // For some reason, the input field does not trigger the change event on testspackages/desktop-client/e2e/page-models/mobile-account-page.ts (1)
37-41: Consider enhancing error handling for balance parsing.The error handling for balance retrieval is good, but the
parseIntcould fail silently if the balance text contains non-numeric characters or currency symbols.Consider this enhanced implementation:
async getBalance() { const balanceText = await this.balance.textContent(); if (!balanceText) { throw new Error('Failed to get balance.'); } - return parseInt(balanceText, 10); + const numericValue = balanceText.replace(/[^0-9.-]/g, ''); + const parsedValue = parseInt(numericValue, 10); + if (isNaN(parsedValue)) { + throw new Error(`Failed to parse balance: ${balanceText}`); + } + return parsedValue; }packages/desktop-client/e2e/page-models/mobile-navigation.ts (1)
46-48: Consider consolidating error handling for bounding box retrieval.Similar error handling code is repeated across multiple methods.
Consider extracting the common logic:
+ private async getBoundingBox(selector: string): Promise<{ width: number; height: number; x: number; y: number }> { + const boundingBox = await this.page.locator(selector).boundingBox(); + if (!boundingBox) { + throw new Error(`Unable to get bounding box of ${selector}.`); + } + return boundingBox; + }Also applies to: 54-56, 72-74
packages/desktop-client/e2e/page-models/account-page.ts (2)
5-12: Consider enhancing TransactionEntry type.The type could be more robust with additional validation.
Consider this enhancement:
type TransactionEntry = { - debit?: string; - credit?: string; + debit?: `${number}` | number; + credit?: `${number}` | number; account?: string; payee?: string; notes?: string; category?: string; + date?: string; // Consider adding date field };
240-246: Consider extending FilterTooltip functionality.The
FilterTooltipclass could be enhanced with methods for common filtering operations.Consider adding methods like:
async applyFilter(value: string): Promise<void> { await this.locator.getByRole('textbox').fill(value); await this.applyButton.click(); } async clearFilter(): Promise<void> { await this.locator.getByRole('textbox').clear(); await this.applyButton.click(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4218.mdis excluded by!**/*.md
📒 Files selected for processing (26)
packages/desktop-client/e2e/budget.mobile.test.ts(2 hunks)packages/desktop-client/e2e/page-models/account-page.ts(11 hunks)packages/desktop-client/e2e/page-models/budget-page.js(0 hunks)packages/desktop-client/e2e/page-models/budget-page.ts(1 hunks)packages/desktop-client/e2e/page-models/close-account-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/configuration-page.ts(2 hunks)packages/desktop-client/e2e/page-models/custom-report-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-account-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-accounts-page.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-budget-page.ts(11 hunks)packages/desktop-client/e2e/page-models/mobile-category-menu-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-navigation.ts(2 hunks)packages/desktop-client/e2e/page-models/mobile-reports-page.js(0 hunks)packages/desktop-client/e2e/page-models/mobile-reports-page.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.ts(1 hunks)packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts(2 hunks)packages/desktop-client/e2e/page-models/navigation.ts(2 hunks)packages/desktop-client/e2e/page-models/reports-page.ts(1 hunks)packages/desktop-client/e2e/page-models/rules-page.ts(4 hunks)packages/desktop-client/e2e/page-models/schedules-page.ts(5 hunks)packages/desktop-client/e2e/page-models/settings-page.ts(2 hunks)packages/desktop-client/e2e/transactions.test.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/e2e/page-models/budget-page.js
- packages/desktop-client/e2e/page-models/mobile-reports-page.js
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/desktop-client/e2e/transactions.test.ts
- packages/desktop-client/e2e/budget.mobile.test.ts
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.ts
- packages/desktop-client/e2e/page-models/close-account-modal.ts
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.ts
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.ts
- packages/desktop-client/e2e/page-models/configuration-page.ts
- packages/desktop-client/e2e/page-models/mobile-reports-page.ts
- packages/desktop-client/e2e/page-models/budget-page.ts
- packages/desktop-client/e2e/page-models/navigation.ts
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.ts
- packages/desktop-client/e2e/page-models/rules-page.ts
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.ts
🔇 Additional comments (27)
packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.ts (4)
1-1: LGTM! Clean type-only imports.Good use of the
typekeyword for importing types, which helps with tree-shaking.
4-11: Well-structured property declarations!Excellent use of:
readonlymodifiers for immutability- Clear, descriptive property names
- Appropriate Playwright types
Line range hint
13-28: Clean constructor implementation with semantic selectors!Good practices:
- Using semantic selectors (
getByRole,getByTestId)- Descriptive button names
- Logical property initialization
Line range hint
30-32: LGTM! Clean async method implementation.The
close()method follows good practices:
- Proper use of async/await
- Clear semantic selectors
- Single responsibility
packages/desktop-client/e2e/page-models/mobile-budget-page.ts (2)
Line range hint
1-10: LGTM! Well-organized imports and class structure.The imports are properly typed and follow a clear organization pattern, separating Playwright types from internal page models.
Line range hint
1-361: Overall excellent TypeScript conversion!The code demonstrates high quality with proper typing, consistent error handling, and good separation of concerns. The suggested improvements are minor and focus on maintainability rather than correctness.
packages/desktop-client/e2e/page-models/schedules-page.ts (6)
1-7: Well-definedScheduleEntrytype enhances type safetyDefining the
ScheduleEntrytype with optional properties improves code clarity and ensures that methods receive correctly structured data.
10-12: Declaringreadonlyproperties with explicit types strengthens code reliabilityBy declaring
page,addNewScheduleButton, andschedulesTableRowasreadonlywith explicit types, you prevent unintended modifications and enhance code robustness.
14-14: Constructor parameter explicitly typed asPageimproves type safetyTyping the
pageparameter asPagein the constructor ensures the class is instantiated with the correct type, enhancing maintainability and reducing potential type errors.
26-26: MethodaddNewScheduleenforces structured inputUpdating
addNewScheduleto acceptdata: ScheduleEntryensures the method receives data that conforms to the expected structure, improving reliability.
38-38: ParameterindexingetNthScheduleRowtyped for claritySpecifying
index: numberingetNthScheduleRowclarifies the expected input type, enhancing code readability.
76-76: Enhanced method flexibility with typed parameters in_performNthActionTyping
index: numberandactionName: string | RegExpin_performNthActionimproves method flexibility and ensures correct usage.packages/desktop-client/e2e/page-models/mobile-accounts-page.ts (6)
1-2: Explicit type imports enhance code clarityImporting
LocatorandPagetypes explicitly from'@playwright/test'improves code readability and type safety.
6-8: Definingreadonlyproperties with explicit types strengthens code reliabilityAdding
readonlypropertiespage,accountList, andaccountListItemswith explicit types prevents unintended mutations and enhances maintainability.
10-10: Constructor parameter explicitly typed asPageimproves type safetyTyping the
pageparameter asPagein the constructor ensures the class is properly instantiated, reducing potential type errors.
17-18: FlexiblewaitFormethod parameters enhance usabilityAllowing
waitForto accept parameters using...options: Parameters<Locator['waitFor']>improves flexibility and ensures options are correctly typed.
Line range hint
24-28: Typingidxparameter enhances method claritySpecifying
idx: numberingetNthAccountclarifies the expected input type, enhancing code readability and preventing errors.
36-38: MethodopenNthAccountparameter typing improves code clarityTyping
idx: numberinopenNthAccountensures proper usage and aligns with TypeScript best practices.packages/desktop-client/e2e/page-models/custom-report-page.ts (5)
1-2: Explicit type imports improve code clarityImporting
LocatorandPagetypes explicitly from'@playwright/test'enhances readability and type safety.
4-8: Addingreadonlyproperties with explicit types strengthens code reliabilityDefining
readonlyproperties forpage,pageContent, and button locators prevents unintended changes and improves code robustness.
10-10: Constructor parameter explicitly typed asPageenhances type safetyTyping the
pageparameter asPagein the constructor ensures correct instantiation and enhances maintainability.
25-25: Flexible parameter type inselectVizenhances method versatilityAllowing
vizNameto bestring | RegExpinselectVizincreases flexibility in matching visualization names.
Line range hint
29-36: Using string literal types inselectModeensures valid inputTyping
modeas'total' | 'time'inselectModerestricts inputs to valid options, enhancing type safety and preventing errors.packages/desktop-client/e2e/page-models/mobile-account-page.ts (2)
6-13: LGTM! Well-structured class properties.The class properties are well-organized with appropriate readonly modifiers and Locator types, following Playwright's best practices.
47-47: Consider adding input validation for search term.The search functionality might benefit from input validation to handle edge cases.
Let's verify how the search functionality is used across the codebase:
packages/desktop-client/e2e/page-models/mobile-navigation.ts (1)
99-102: Well-implemented generic type constraint.The generic type parameter with the
waitForconstraint ensures type safety while maintaining flexibility for different page models.packages/desktop-client/e2e/page-models/account-page.ts (1)
192-195: Consider adding validation for transaction fields.The
_fillTransactionFieldsmethod could benefit from input validation before attempting to fill the fields.Let's check for any validation patterns used elsewhere in the codebase:
|
/update-vrt |
* Dummy commit * Delete js snapshots * Move extended expect and test to fixtures * Fix wrong commit * Update VRT * Dummy commit to run GH actions * Convert test page models to TS * Release notes * Fix typecheck errors * New page models to TS * Fix typecheck error * Fix page name * Put awaits on getTableTotals --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
No description provided.