test: test Deployment and Tx Fee authorizations #2109
Conversation
WalkthroughThis PR introduces comprehensive UI test coverage for spending authorization flows with accessibility improvements across authorization components. Updates test infrastructure to support wallet extension interactions, adds helper functions and page objects following the page object model pattern, refactors fixture setup, and extends wallet operation handling. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant Setup as Setup Helper
participant LeapExt as LeapExt Instance
participant AuthPage as AuthorizationsPage
participant Extension as Wallet Extension
participant App as Deploy Web App
Test->>Setup: Initialize LeapExt + create wallet
Setup->>LeapExt: new LeapExt(context, page)
Setup->>LeapExt: createWallet()
LeapExt->>Extension: Trigger wallet creation
Setup->>App: Navigate to AuthorizationsPage
Setup->>LeapExt: authorizeSpending (initial)
rect rgb(200, 220, 255)
Note over Test,AuthPage: Test Scenario: Authorize Spending
Test->>AuthPage: goto() & clickGrantButton()
Test->>AuthPage: authorizeSpending(address, label)
AuthPage->>App: Fill Spending Limit & Grantee
AuthPage->>App: Click Grant
Test->>AuthPage: Verify address in list
end
rect rgb(200, 220, 255)
Note over Test,AuthPage: Test Scenario: Edit Spending
Test->>AuthPage: editSpending(address, label)
AuthPage->>App: Locate row by shortened address
AuthPage->>App: Click Edit Authorization
AuthPage->>App: Update Spending Limit
AuthPage->>App: Click Grant
Test->>AuthPage: Verify updated amount in list
end
rect rgb(200, 220, 255)
Note over Test,AuthPage: Test Scenario: Revoke Spending
Test->>AuthPage: revokeSpending(address, label)
AuthPage->>App: Locate row by shortened address
AuthPage->>App: Click Revoke Authorization
AuthPage->>App: Confirm revocation
Test->>AuthPage: Verify address removed from list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
apps/deploy-web/src/components/authorizations/GrantModal.tsx (1)
192-192: ARIA labels on inputs with existing label props.The
aria-labelattributes on these FormInput components duplicate the visiblelabelprop values. If FormInput properly associates the label with the input (via htmlFor), the aria-label is redundant since the label text already provides the accessible name. However, this is not harmful and may provide defensive accessibility if the label association isn't perfect.Also applies to: 204-204
apps/deploy-web/src/components/authorizations/AllowanceModal.tsx (1)
147-147: ARIA labels on inputs with existing label props.Similar to GrantModal.tsx, these
aria-labelattributes duplicate the visiblelabelprop values. While potentially redundant if FormInput properly associates labels with inputs, this provides defensive accessibility and maintains consistency with the other authorization modals.Also applies to: 159-159
apps/deploy-web/src/components/authorizations/Authorizations.tsx (2)
226-229: ARIA label duplicates visible button text.This button already has visible text content ("Authorize Spend"), which automatically provides the accessible name. The aria-label duplicates this visible text, making it redundant but not harmful. Consider removing the aria-label since the visible text is sufficient for accessibility.
338-349: ARIA label duplicates visible button text.Similar to the "Authorize Spend" button, this button has visible text content ("Authorize Fee Spend") that already provides an accessible name. The aria-label is redundant but not harmful.
apps/deploy-web/tests/ui/authorize-spending.spec.ts (2)
17-53: Consider restructuring to follow test guidelines.The current parametric test approach with
runAuthorizationTestis functional but doesn't align with the coding guidelines which specify:
- "Use
setupfunction instead ofbeforeEachin test files"- "
setupfunction must be at the bottom of the rootdescribeblock in test files"The current structure calls
setup()directly in each test rather than using abeforeEach-style pattern, and thesetupfunction is at module level rather than within adescribeblock.Consider this structure for better guideline compliance:
test.describe("Deployment Authorizations", () => { test("can authorize spending", async ({ page, context, extensionId }) => { test.setTimeout(5 * 60 * 1000); const { authorizationsPage, address } = setup({ page, context, extensionId }); const buttonLabel: AuthorizeButtonLabel = "Authorize Spend"; const listLabel: AuthorizationListLabel = "Deployment Authorization List"; await authorizationsPage.authorizeSpending(address, buttonLabel); const shortenedAddress = shortenAddress(address); const grantList = authorizationsPage.page.getByLabel(listLabel); await expect(grantList.locator("tr", { hasText: shortenedAddress })).toBeVisible(); }); // ... other tests function setup({ page, context, extensionId }: SetupProps) { // setup logic } });Alternatively, if you want to maintain the parametric approach, document why this pattern was chosen over the standard guidelines.
19-27: Test timeout set per test instead of using fixture configuration.The 5-minute timeout is set individually in each test case, but the fixture already configures this timeout (line 77 in context-with-extension.ts). This creates redundancy.
Consider removing the per-test timeout since the fixture already handles it:
test("can authorize spending", async ({ page, context, extensionId }) => { - test.setTimeout(5 * 60 * 1000); - const { authorizationsPage, address } = await setup({ page, context, extensionId, buttonLabel });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/deploy-web/src/components/authorizations/AllowanceIssuedRow.tsx(1 hunks)apps/deploy-web/src/components/authorizations/AllowanceModal.tsx(2 hunks)apps/deploy-web/src/components/authorizations/Authorizations.tsx(2 hunks)apps/deploy-web/src/components/authorizations/DeploymentGrantTable.tsx(2 hunks)apps/deploy-web/src/components/authorizations/FeeGrantTable.tsx(1 hunks)apps/deploy-web/src/components/authorizations/GrantModal.tsx(2 hunks)apps/deploy-web/tests/ui/authorize-spending.spec.ts(1 hunks)apps/deploy-web/tests/ui/change-wallets.spec.ts(1 hunks)apps/deploy-web/tests/ui/disconnect-wallet.spec.ts(1 hunks)apps/deploy-web/tests/ui/fixture/context-with-extension.ts(2 hunks)apps/deploy-web/tests/ui/fixture/testing-helpers.ts(2 hunks)apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx(1 hunks)apps/deploy-web/tests/ui/pages/LeapExt.tsx(1 hunks)apps/deploy-web/tests/ui/uiState/isWalletConnected.ts(1 hunks)packages/ui/components/custom/address.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/authorizations/Authorizations.tsxapps/deploy-web/tests/ui/pages/LeapExt.tsxapps/deploy-web/tests/ui/fixture/testing-helpers.tsapps/deploy-web/src/components/authorizations/GrantModal.tsxpackages/ui/components/custom/address.tsxapps/deploy-web/tests/ui/change-wallets.spec.tsapps/deploy-web/src/components/authorizations/AllowanceIssuedRow.tsxapps/deploy-web/src/components/authorizations/FeeGrantTable.tsxapps/deploy-web/tests/ui/uiState/isWalletConnected.tsapps/deploy-web/tests/ui/disconnect-wallet.spec.tsapps/deploy-web/src/components/authorizations/AllowanceModal.tsxapps/deploy-web/src/components/authorizations/DeploymentGrantTable.tsxapps/deploy-web/tests/ui/pages/AuthorizationsPage.tsxapps/deploy-web/tests/ui/authorize-spending.spec.tsapps/deploy-web/tests/ui/fixture/context-with-extension.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/authorizations/Authorizations.tsxapps/deploy-web/tests/ui/pages/LeapExt.tsxapps/deploy-web/tests/ui/fixture/testing-helpers.tsapps/deploy-web/src/components/authorizations/GrantModal.tsxpackages/ui/components/custom/address.tsxapps/deploy-web/tests/ui/change-wallets.spec.tsapps/deploy-web/src/components/authorizations/AllowanceIssuedRow.tsxapps/deploy-web/src/components/authorizations/FeeGrantTable.tsxapps/deploy-web/tests/ui/uiState/isWalletConnected.tsapps/deploy-web/tests/ui/disconnect-wallet.spec.tsapps/deploy-web/src/components/authorizations/AllowanceModal.tsxapps/deploy-web/src/components/authorizations/DeploymentGrantTable.tsxapps/deploy-web/tests/ui/pages/AuthorizationsPage.tsxapps/deploy-web/tests/ui/authorize-spending.spec.tsapps/deploy-web/tests/ui/fixture/context-with-extension.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/deploy-web/tests/ui/change-wallets.spec.tsapps/deploy-web/tests/ui/disconnect-wallet.spec.tsapps/deploy-web/tests/ui/authorize-spending.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
PR: akash-network/console#2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/deploy-web/tests/ui/change-wallets.spec.tsapps/deploy-web/tests/ui/disconnect-wallet.spec.tsapps/deploy-web/tests/ui/authorize-spending.spec.ts
🧬 Code graph analysis (6)
apps/deploy-web/tests/ui/change-wallets.spec.ts (1)
apps/deploy-web/tests/ui/pages/LeapExt.tsx (1)
LeapExt(9-46)
apps/deploy-web/tests/ui/disconnect-wallet.spec.ts (1)
apps/deploy-web/tests/ui/pages/LeapExt.tsx (1)
LeapExt(9-46)
apps/deploy-web/src/components/authorizations/AllowanceModal.tsx (1)
packages/ui/components/input.tsx (1)
FormInput(123-123)
apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx (2)
apps/deploy-web/tests/ui/fixture/test-env.config.ts (1)
testEnvConfig(12-16)packages/ui/components/custom/address.tsx (1)
shortenAddress(11-13)
apps/deploy-web/tests/ui/authorize-spending.spec.ts (6)
apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx (3)
AuthorizeButtonLabel(6-6)AuthorizationListLabel(7-7)AuthorizationsPage(9-55)apps/deploy-web/tests/ui/fixture/context-with-extension.ts (2)
test(12-79)expect(81-81)packages/ui/components/custom/address.tsx (1)
shortenAddress(11-13)apps/deploy-web/tests/ui/pages/LeapExt.tsx (1)
LeapExt(9-46)apps/deploy-web/tests/ui/fixture/testing-helpers.ts (1)
clickCopyAddressButton(36-45)apps/deploy-web/tests/ui/fixture/wallet-setup.ts (1)
getExtensionPage(15-27)
apps/deploy-web/tests/ui/fixture/context-with-extension.ts (3)
apps/deploy-web/tests/ui/fixture/wallet-setup.ts (4)
getExtensionPage(15-27)setupWallet(29-34)connectWalletViaLeap(46-57)awaitWalletAndApprove(59-63)apps/deploy-web/tests/ui/fixture/base-test.ts (1)
injectUIConfig(16-25)apps/deploy-web/tests/ui/actions/selectChainNetwork.ts (1)
selectChainNetwork(6-20)
⏰ 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). (5)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (15)
apps/deploy-web/tests/ui/uiState/isWalletConnected.ts (1)
7-7: Document why 30s timeout is needed for authorization workflow.The timeout increase from 10s to 30s is intentional for your authorization workflow testing with wallet extensions. However, this is significantly longer than other wallet operations in the codebase (which use 20s max) and lacks explanation. Add a code comment clarifying why the extended timeout is necessary, or verify that this timeout is appropriate for your CI environment. Consider whether the 30s timeout addresses a real performance need or masks underlying test flakiness.
apps/deploy-web/src/components/authorizations/FeeGrantTable.tsx (1)
66-66: LGTM!Adding an aria-label to the Table component improves accessibility for screen reader users by providing context about the table's purpose.
apps/deploy-web/tests/ui/disconnect-wallet.spec.ts (1)
6-7: LGTM!The variable rename from
frontPagetoextensionbetter reflects that this is a LeapExt instance representing wallet extension interactions, improving code clarity.apps/deploy-web/tests/ui/change-wallets.spec.ts (1)
9-11: LGTM!Consistent with the refactoring in disconnect-wallet.spec.ts, the rename from
frontPagetoextensionimproves code clarity and naming consistency across the test suite.apps/deploy-web/src/components/authorizations/AllowanceIssuedRow.tsx (1)
56-59: LGTM!Adding aria-labels to these icon-only buttons is essential for accessibility. Screen reader users will now understand the purpose of these action buttons.
apps/deploy-web/src/components/authorizations/DeploymentGrantTable.tsx (2)
133-136: LGTM!Adding aria-labels to these icon-only Edit and Revoke buttons provides essential accessible names for screen reader users.
167-167: LGTM!The aria-label on the Table component improves accessibility by providing screen reader users with context about the table's purpose.
packages/ui/components/custom/address.tsx (1)
28-28: LGTM! Good refactor to use the centralized helper.The change correctly delegates address formatting to the new
shortenAddresshelper, maintaining consistent behavior while improving code reusability.apps/deploy-web/tests/ui/fixture/context-with-extension.ts (2)
30-31: LGTM! Clipboard permissions support the new test helpers.The addition of clipboard read/write permissions enables the
clickCopyAddressButtonhelper to read clipboard contents during tests.
46-78: LGTM! Appropriate fixture scope and timeout configuration.The tuple format correctly specifies
scope: "test"(isolated per test) and a 5-minute timeout, which is reasonable given the fixture performs wallet setup, network selection, and wallet connection. The fallback logic for non-functional networks is a good defensive measure.apps/deploy-web/tests/ui/fixture/testing-helpers.ts (2)
18-18: LGTM! Simplified by removing unnecessary intermediate await.The direct return of the fill operation is correct and more concise.
36-45: LGTM! Clipboard reading logic is correct.The helper properly handles the null case from
getCopyAddressButtonand uses the Playwright evaluate API to read clipboard contents.apps/deploy-web/tests/ui/authorize-spending.spec.ts (1)
62-77: Verify the setup creates a second wallet as intended.The
setupfunction callsextension.createWallet(extensionId)on line 65. Based on learnings, the first wallet is automatically created during fixture setup, so this creates a second wallet for authorization testing.Confirm this is intentional behavior - creating a second wallet to test authorizations between different wallet addresses. If so, consider adding a comment explaining this:
const setup = async ({ page, context, extensionId, buttonLabel }: SetupProps) => { const extension = new LeapExt(context, page); const address = await clickCopyAddressButton(await getExtensionPage(context, extensionId)); // Create a second wallet to test cross-wallet authorizations await extension.createWallet(extensionId); // ... rest of setup };Based on learnings
apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx (2)
6-7: LGTM! Clear type definitions for authorization UI elements.The string literal union types provide type safety and document the exact labels used in the UI.
32-40: Good use of shortenAddress for consistent address matching.The page object correctly uses
shortenAddress()to format addresses for UI element selection, ensuring consistency with how addresses are displayed in the authorization lists.
| async authorizeSpending(address: string, buttonLabel: AuthorizeButtonLabel) { | ||
| await this.page.getByLabel(buttonLabel).click(); | ||
| await this.page.getByLabel("Spending Limit").fill("5"); | ||
| await this.page.getByLabel("Grantee Address").fill(address); | ||
|
|
||
| this.context.on("page", this.acceptTransaction.bind(this)); | ||
| await this.clickGrantButton(); | ||
| } |
There was a problem hiding this comment.
Critical: Event listener won't be unregistered due to mismatched function references.
The same issue identified in LeapExt.tsx applies here. Each call to bind(this) creates a new function reference, so the listener registered on line 28 won't be properly removed by the off() call in acceptTransaction() (line 53).
Apply this diff to fix the binding issue:
export class AuthorizationsPage {
+ private boundAcceptTransaction?: () => Promise<void>;
+
constructor(
readonly context: Context,
readonly page: Page
) {}
// ... other methods
async authorizeSpending(address: string, buttonLabel: AuthorizeButtonLabel) {
await this.page.getByLabel(buttonLabel).click();
await this.page.getByLabel("Spending Limit").fill("5");
await this.page.getByLabel("Grantee Address").fill(address);
- this.context.on("page", this.acceptTransaction.bind(this));
+ this.boundAcceptTransaction = this.acceptTransaction.bind(this);
+ this.context.on("page", this.boundAcceptTransaction);
await this.clickGrantButton();
}
async editSpending(address: string, listLabel: AuthorizationListLabel) {
const shortenedAddress = shortenAddress(address);
await this.page.getByLabel(listLabel).locator("tr", { hasText: shortenedAddress }).getByLabel("Edit Authorization").click();
await this.page.getByLabel("Spending Limit").fill("10");
- this.context.on("page", this.acceptTransaction.bind(this));
+ this.boundAcceptTransaction = this.acceptTransaction.bind(this);
+ this.context.on("page", this.boundAcceptTransaction);
await this.clickGrantButton();
}
async revokeSpending(address: string, listLabel: AuthorizationListLabel) {
const shortenedAddress = shortenAddress(address);
await this.page.getByLabel(listLabel).locator("tr", { hasText: shortenedAddress }).getByLabel("Revoke Authorization").click();
- this.context.on("page", this.acceptTransaction.bind(this));
+ this.boundAcceptTransaction = this.acceptTransaction.bind(this);
+ this.context.on("page", this.boundAcceptTransaction);
await this.page.getByTestId("confirm-button").click();
}
async acceptTransaction() {
await this.page.waitForLoadState("domcontentloaded");
await this.page.getByTestId("btn-connect-wallet").click();
- this.context.off("page", this.acceptTransaction.bind(this));
+ if (this.boundAcceptTransaction) {
+ this.context.off("page", this.boundAcceptTransaction);
+ this.boundAcceptTransaction = undefined;
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx around lines 23 to 30,
the event listener is registered with this.acceptTransaction.bind(this) which
creates a new function reference and prevents removal later; create a persistent
bound handler (e.g., store const boundAcceptTransaction =
this.acceptTransaction.bind(this) in a local variable or class property) and use
that same boundAcceptTransaction when calling this.context.on(...) and ensure
acceptTransaction() uses this.context.off(boundAcceptTransaction) to unregister
the exact same reference.
| async acceptTransaction() { | ||
| await this.page.waitForLoadState("domcontentloaded"); | ||
| await this.page.getByTestId("btn-connect-wallet").click(); | ||
| this.context.off("page", this.acceptTransaction.bind(this)); | ||
| } |
There was a problem hiding this comment.
Critical: Event listener won't be unregistered due to mismatched function references.
Each call to bind(this) creates a new function reference. When you register the listener with this.context.on("page", this.acceptTransaction.bind(this)) and then try to unregister with this.context.off("page", this.acceptTransaction.bind(this)), you're using different function references, so the listener remains attached.
Apply this diff to fix the binding issue:
+ private boundAcceptTransaction?: () => Promise<void>;
+
async acceptTransaction() {
await this.page.waitForLoadState("domcontentloaded");
await this.page.getByTestId("btn-connect-wallet").click();
- this.context.off("page", this.acceptTransaction.bind(this));
+ if (this.boundAcceptTransaction) {
+ this.context.off("page", this.boundAcceptTransaction);
+ this.boundAcceptTransaction = undefined;
+ }
}Then when registering the listener (where this method is called from), use:
this.boundAcceptTransaction = this.acceptTransaction.bind(this);
this.context.on("page", this.boundAcceptTransaction);Note: I see this same pattern is used in AuthorizationsPage.tsx. The same fix is needed there as well.
🤖 Prompt for AI Agents
In apps/deploy-web/tests/ui/pages/LeapExt.tsx around lines 41 to 45, the
off("page", this.acceptTransaction.bind(this)) call uses a different function
reference than the one passed to on(), so the listener is never removed; fix by
creating and using a stored bound function: add an instance property (e.g.,
this.boundAcceptTransaction = this.acceptTransaction.bind(this)) where the
listener is registered and use this.context.on("page",
this.boundAcceptTransaction) and here replace the off call with
this.context.off("page", this.boundAcceptTransaction); apply the same pattern to
AuthorizationsPage.tsx where the same bind/unbind mistake exists.
| export const shortenAddress = (address: string) => { | ||
| return [address?.slice(0, 8), "...", address?.slice(address?.length - 5)].join(""); | ||
| }; |
There was a problem hiding this comment.
Guard against null/undefined address values.
The optional chaining on address?.slice() will return undefined if address is null/undefined, but the join("") will still execute, producing output like "undefined...undefined". Consider adding an early return or guard condition.
Apply this diff to handle null/undefined addresses:
export const shortenAddress = (address: string) => {
+ if (!address) return "";
return [address?.slice(0, 8), "...", address?.slice(address?.length - 5)].join("");
};Alternatively, remove the optional chaining if the function expects a valid string:
-export const shortenAddress = (address: string) => {
- return [address?.slice(0, 8), "...", address?.slice(address?.length - 5)].join("");
+export const shortenAddress = (address: string): string => {
+ return `${address.slice(0, 8)}...${address.slice(-5)}`;
};📝 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.
| export const shortenAddress = (address: string) => { | |
| return [address?.slice(0, 8), "...", address?.slice(address?.length - 5)].join(""); | |
| }; | |
| export const shortenAddress = (address: string): string => { | |
| return `${address.slice(0, 8)}...${address.slice(-5)}`; | |
| }; |
🤖 Prompt for AI Agents
In packages/ui/components/custom/address.tsx around lines 11 to 13, the
shortenAddress function uses optional chaining on address.slice which yields
"undefined...undefined" when address is null/undefined; add an early guard that
returns an empty string (or the original value) when address is falsy, or
explicitly throw/validate if a string is required, then remove the optional
chaining and safely call slice on the confirmed string so join("") cannot
produce "undefined" parts.
There was a problem hiding this comment.
@jzsfkzm , makes sense to apply this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| step={0.000001} | ||
| max={denomData?.max} | ||
| startIcon={<span className="pl-2 text-xs">{denomData?.label}</span>} | ||
| aria-label="Spending Limit" |
There was a problem hiding this comment.
issue(blocking): this element has label="Spending label", you can issue page.getByLabel().But it requires to fix label linking in input.tsx
- {label && (formField.id ? <FormLabel>{label}</FormLabel> : <Label htmlFor={`${finalId}-input`}>{label}</Label>)}
+ {label && (formField.id ? <FormLabel htmlFor={`${finalId}-input`}>{label}</FormLabel> : <Label htmlFor={`${finalId}-input`}>{label}</Label>)}| name="granteeAddress" | ||
| render={({ field }) => { | ||
| return <FormInput {...field} type="text" label="Grantee Address" disabled={!!editingAllowance} />; | ||
| return <FormInput {...field} type="text" label="Grantee Address" disabled={!!editingAllowance} aria-label="Grantee Address" />; |
There was a problem hiding this comment.
| address && ( | ||
| <div className="md:ml-4"> | ||
| <Button onClick={onCreateNewGrant} color="secondary" variant="default" type="button" size="sm"> | ||
| <Button onClick={onCreateNewGrant} color="secondary" variant="default" type="button" size="sm" aria-label="Authorize Spend"> |
There was a problem hiding this comment.
issue(blocking): aria-label is redundant, just use getByRole('button', { name: /Authorize Spend/ })
| className="md:ml-4" | ||
| type="button" | ||
| size="sm" | ||
| aria-label="Authorize Fee Spend" |
There was a problem hiding this comment.
issue(blocking): redundant find button by role and name
| max={denomData?.max} | ||
| startIcon={<span className="pl-2 text-xs">{denomData?.label}</span>} | ||
| className="ml-4 flex-grow" | ||
| aria-label="Spending Limit" |
There was a problem hiding this comment.
| name="granteeAddress" | ||
| render={({ field }) => { | ||
| return <FormInput {...field} type="text" label="Grantee Address" disabled={!!editingGrant} />; | ||
| return <FormInput {...field} type="text" label="Grantee Address" disabled={!!editingGrant} aria-label="Grantee Address" />; |
There was a problem hiding this comment.
| async acceptTransaction() { | ||
| await this.page.waitForLoadState("domcontentloaded"); | ||
| await this.page.getByTestId("btn-connect-wallet").click(); | ||
| this.context.off("page", this.acceptTransaction.bind(this)); |
There was a problem hiding this comment.
issue(blocking): this doesn't work because .bind creates a new function. you have 2 alternative options to context.on('page', ...):
| export const shortenAddress = (address: string) => { | ||
| return [address?.slice(0, 8), "...", address?.slice(address?.length - 5)].join(""); | ||
| }; |
There was a problem hiding this comment.
@jzsfkzm , makes sense to apply this
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/ui/components/custom/address.tsx (1)
11-13: Add guard for null/undefined address values (unresolved from past review).The function signature expects a valid string, but there's no runtime validation. A previous review suggested adding a guard condition, and a maintainer agreed it should be applied. The change from optional chaining indicates an intent to require valid strings, but without a guard, null/undefined values will cause runtime errors.
Apply this diff to add proper validation:
-export const shortenAddress = (address: string) => { - return `${address.slice(0, 8)}...${address.slice(-5)}`; +export const shortenAddress = (address: string): string => { + if (!address) return ""; + return `${address.slice(0, 8)}...${address.slice(-5)}`; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/deploy-web/src/components/authorizations/Authorizations.tsx(1 hunks)apps/deploy-web/tests/ui/authorize-spending.spec.ts(1 hunks)apps/deploy-web/tests/ui/fixture/context-with-extension.ts(2 hunks)apps/deploy-web/tests/ui/fixture/testing-helpers.ts(2 hunks)apps/deploy-web/tests/ui/fixture/wallet-setup.ts(1 hunks)apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx(1 hunks)apps/deploy-web/tests/ui/pages/LeapExt.tsx(2 hunks)packages/ui/components/custom/address.tsx(2 hunks)packages/ui/components/input.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/deploy-web/src/components/authorizations/Authorizations.tsx
- apps/deploy-web/tests/ui/authorize-spending.spec.ts
- apps/deploy-web/tests/ui/fixture/testing-helpers.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/tests/ui/fixture/wallet-setup.tsapps/deploy-web/tests/ui/pages/LeapExt.tsxapps/deploy-web/tests/ui/fixture/context-with-extension.tspackages/ui/components/input.tsxpackages/ui/components/custom/address.tsxapps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/tests/ui/fixture/wallet-setup.tsapps/deploy-web/tests/ui/pages/LeapExt.tsxapps/deploy-web/tests/ui/fixture/context-with-extension.tspackages/ui/components/input.tsxpackages/ui/components/custom/address.tsxapps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
PR: akash-network/console#2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/deploy-web/tests/ui/fixture/wallet-setup.tsapps/deploy-web/tests/ui/pages/LeapExt.tsx
🧬 Code graph analysis (4)
apps/deploy-web/tests/ui/pages/LeapExt.tsx (1)
apps/deploy-web/tests/ui/fixture/wallet-setup.ts (1)
approveWalletOperation(68-95)
apps/deploy-web/tests/ui/fixture/context-with-extension.ts (3)
apps/deploy-web/tests/ui/fixture/wallet-setup.ts (4)
getExtensionPage(15-27)setupWallet(29-34)connectWalletViaLeap(46-57)awaitWalletAndApprove(59-66)apps/deploy-web/tests/ui/fixture/base-test.ts (1)
injectUIConfig(16-25)apps/deploy-web/tests/ui/actions/selectChainNetwork.ts (1)
selectChainNetwork(6-20)
packages/ui/components/input.tsx (2)
packages/ui/components/form.tsx (1)
FormLabel(133-133)packages/ui/components/label.tsx (1)
Label(17-17)
apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx (2)
apps/deploy-web/tests/ui/fixture/test-env.config.ts (1)
testEnvConfig(12-16)packages/ui/components/custom/address.tsx (1)
shortenAddress(11-13)
⏰ 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). (5)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (8)
apps/deploy-web/tests/ui/pages/LeapExt.tsx (2)
2-2: LGTM! Type imports aligned with usage.The updates correctly import
BrowserContextandapproveWalletOperation, and the constructor parameter type is now explicit and consistent with Playwright's types.Also applies to: 7-7, 11-11
41-44: LGTM! Clean implementation avoids event listener complexity.This straightforward implementation waits for the popup page and approves the wallet operation without the event listener registration/cleanup pattern that was problematic in earlier iterations. The simpler approach is more maintainable.
apps/deploy-web/tests/ui/fixture/context-with-extension.ts (1)
30-31: LGTM! Clipboard permissions support copy operations in tests.The clipboard permissions are appropriate for E2E tests that need to verify address copying functionality.
apps/deploy-web/tests/ui/pages/AuthorizationsPage.tsx (4)
1-13: LGTM! Clean page object setup with type-safe label constraints.The type aliases for
AuthorizeButtonLabelandAuthorizationListLabelprovide compile-time safety for UI interaction selectors, following best practices for the page object pattern.
23-28: LGTM! Authorization flow correctly delegates wallet approval to caller.The method performs the UI interactions for setting up authorization without attempting to handle the wallet approval popup. Past reviews flagged event listener issues, but the current implementation correctly separates concerns by letting the test orchestrate the approval step externally (as seen in LeapExt.acceptTransaction).
30-35: LGTM! Edit flow correctly uses shortened address for matching.The method properly shortens the address to match the displayed format in the table, ensuring reliable element location.
37-41: LGTM! Revoke flow follows consistent pattern.The revocation flow correctly implements a two-step confirmation pattern and matches rows using the shortened address format.
apps/deploy-web/tests/ui/fixture/wallet-setup.ts (1)
59-66: All callers have been updated with theextensionIdparameter.The verification confirms that the only caller at
apps/deploy-web/tests/ui/fixture/context-with-extension.ts:71is already passing all three required parameters:context,page, andextensionId. The implementation correctly uses this parameter in thegetExtensionPage()call. No issues found.
| page: [ | ||
| async ({ context, extensionId }, use) => { | ||
| try { | ||
| await context.waitForEvent("page", { timeout: 5000 }); | ||
| } catch { | ||
| // ignore timeout error | ||
| } | ||
|
|
||
| const extPage = await getExtensionPage(context, extensionId); | ||
| const extPage = await getExtensionPage(context, extensionId); | ||
|
|
||
| await setupWallet(context, extPage); | ||
| await extPage.close(); | ||
| const page = await context.newPage(); | ||
| await injectUIConfig(page); | ||
| await setupWallet(context, extPage); | ||
| await extPage.close(); | ||
| const page = await context.newPage(); | ||
| await injectUIConfig(page); | ||
|
|
||
| if (testEnvConfig.NETWORK_ID !== "mainnet") { | ||
| try { | ||
| await page.goto(testEnvConfig.BASE_URL); | ||
| await connectWalletViaLeap(context, page); | ||
| await selectChainNetwork(page, testEnvConfig.NETWORK_ID); | ||
| await connectWalletViaLeap(context, page); | ||
| } catch { | ||
| // Fallback in case the default network is non-functional. | ||
| // E.g., during network upgrade when sandbox is already on a different version from mainnet | ||
| await page.goto(`${testEnvConfig.BASE_URL}?network=${testEnvConfig.NETWORK_ID}`); | ||
| await awaitWalletAndApprove(context, page); | ||
| if (testEnvConfig.NETWORK_ID !== "mainnet") { | ||
| try { | ||
| await page.goto(testEnvConfig.BASE_URL); | ||
| await connectWalletViaLeap(context, page); | ||
| await selectChainNetwork(page, testEnvConfig.NETWORK_ID); | ||
| await connectWalletViaLeap(context, page); | ||
| } catch { | ||
| // Fallback in case the default network is non-functional. | ||
| // E.g., during network upgrade when sandbox is already on a different version from mainnet | ||
| await page.goto(`${testEnvConfig.BASE_URL}?network=${testEnvConfig.NETWORK_ID}`); | ||
| await awaitWalletAndApprove(context, page, extensionId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await use(page); | ||
| } | ||
| await use(page); | ||
| }, | ||
| { scope: "test", timeout: 5 * 60 * 1000 } | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the 5-minute timeout is necessary.
The page fixture now has explicit scope and timeout configuration. The test scope is appropriate for isolation, but the 5-minute timeout seems quite long for fixture setup.
Is the 5-minute timeout necessary for your test environment? Consider whether a shorter timeout (e.g., 2-3 minutes) would be sufficient, as excessively long timeouts can mask real issues with slow setup operations.
🌐 Web query:
What is a typical timeout for Playwright page fixtures in E2E tests?
💡 Result:
- Default: Playwright’s per-test timeout is 30,000 ms (30s), and fixtures share that timeout by default. [1]
- Common practice: keep quick page fixtures within the default 30s; for slower (worker-scoped or heavy) page fixtures people commonly set a separate fixture timeout of 60,000 ms (60s) or similar. Example:
{ timeout: 60000 }on the fixture. [1][2]
References:
[1] Playwright Test — Timeouts (defaults and fixture behavior).
[2] Playwright docs/examples showing fixture timeout usage (example uses 60000 ms).
Reduce fixture timeout from 5 minutes to align with best practices.
The 5-minute timeout (300,000 ms) is significantly excessive. Playwright's default per-test timeout is 30 seconds, and even for slower or heavy fixtures, common practice is to use a separate fixture timeout of around 60 seconds.
Set the timeout to 60,000 ms (1 minute) or 90,000 ms if the wallet setup consistently requires that duration. Overly long timeouts mask real issues with slow operations.
🤖 Prompt for AI Agents
In apps/deploy-web/tests/ui/fixture/context-with-extension.ts around lines 46 to
78, the fixture timeout is set to 5 * 60 * 1000 (300000 ms) which is excessive;
change the timeout value in the fixture options to 60000 (60,000 ms) to align
with best practices (or 90000 if you have measured that the wallet setup
consistently needs more time), keeping the same scope and behavior otherwise.
| return ( | ||
| <div className={cn("space-y-1", className)}> | ||
| {label && (formField.id ? <FormLabel>{label}</FormLabel> : <Label htmlFor={`${finalId}-input`}>{label}</Label>)} | ||
| {label && (formField.id ? <FormLabel htmlFor={`${finalId}-input`}>{label}</FormLabel> : <Label htmlFor={`${finalId}-input`}>{label}</Label>)} |
There was a problem hiding this comment.
🧩 Analysis chain
Good accessibility improvement; verify consistency with Textarea.
The addition of {label} as children to FormLabel and the htmlFor attribute improves accessibility by ensuring the label renders correctly and explicitly links to the input.
However, the Textarea component at line 107 uses FormLabel without the htmlFor attribute. For consistency, verify whether Textarea's FormLabel should also include htmlFor={${formField.id ? formField.id : id}-input} to match this pattern.
🏁 Script executed:
#!/bin/bash
# Verify if FormLabel in other similar components should also have htmlFor attribute
rg -n -C3 -P '<FormLabel[^>]*>.*?</FormLabel>' --type=tsx --type=tsLength of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Alternative approach: find and examine the input.tsx file directly
echo "=== Examining input.tsx file ==="
fd -e tsx -e ts input.tsx
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for FormLabel patterns using file glob instead of type
rg -n -C3 '<FormLabel' packages/ui/ Length of output: 1199
🏁 Script executed:
#!/bin/bash
# Look for all tsx files in the ui components directory
fd -e tsx packages/ui/components/Length of output: 397
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the full input.tsx file to understand context around both components
cat packages/ui/components/input.tsx
Length of output: 4778
Add htmlFor attribute to Textarea's FormLabel for consistency with Input component.
The accessibility improvement at line 59 (adding htmlFor to Input's FormLabel) is good. However, Textarea at line 107 uses FormLabel without the htmlFor attribute. Both components follow identical patterns when formField.id exists, so Textarea's FormLabel should also include htmlFor={${formField.id ? formField.id : id}-input} to maintain consistent accessibility.
Update line 107:
{label && (formField.id ? <FormLabel htmlFor={`${formField.id ? formField.id : id}-input`}>{label}</FormLabel> : <Label htmlFor={`${id}-input`}>{label}</Label>)}
🤖 Prompt for AI Agents
In packages/ui/components/input.tsx around lines 59 and 107, the Textarea
component's FormLabel is missing the htmlFor attribute (unlike the Input
component), so add htmlFor using the same id expression as Input: set
FormLabel's htmlFor to `${formField.id ? formField.id : id}-input` and keep the
existing Label fallback using `${id}-input`; this ensures consistent accessible
labeling for both Input and Textarea.
3 participants
refs #2006
Summary by CodeRabbit
Release Notes
Accessibility
Bug Fixes
Tests