feat(12.5): MFA polishing, UAT & E2E testing#131
Merged
Conversation
Phase 12.5: MFA Polishing, UAT & E2E Testing - 3 plan(s) in 2 wave(s) - 2 parallel (wave 1), 1 sequential (wave 2) - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation - Merge tabbed UI (LINKED METHODS | SECURITY) from orphaned Settings.tsx into SettingsPage.tsx - Add SecurityTab import and render within security tab panel - Preserve AppShell wrapper, auth guard, and VaultExport section below tabs - Add full ARIA tablist/tab/tabpanel attributes for accessibility - Delete orphaned Settings.tsx (no longer imported anywhere) Resolves ISSUE-004 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Page - Add @johanneskares/wallet-mock and viem as e2e devDependencies - Extend LoginPage page object with wallet-specific selectors and methods - New methods: clickWalletLogin, getWalletConnectors, selectWalletConnector, cancelWalletLogin, getWalletError, isWalletConnectorListVisible, getWalletStatus, isWalletLoginButtonVisible, isWalletLoginButtonEnabled, isNoWalletProvidersVisible Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 1/1 - Merge Settings.tsx tab UI into SettingsPage.tsx and remove orphan SUMMARY: .planning/phases/12.5-mfa-polishing-uat-e2e/12.5-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…3 provider - TC09: Happy path - mock wallet discovered via EIP-6963, SIWE flow initiated - TC10: Cancel - connector list dismissed, button returns to idle - TC11: No wallet - handles missing providers gracefully - TC12: Error handling - intercepted nonce endpoint 500 shows error, allows retry - Uses @johanneskares/wallet-mock with Hardhat account #0 for real ECDSA signatures - Tests validate frontend UI flow independent of Core Kit availability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Install wallet-mock and add wallet selectors to LoginPage - Create wallet-login.spec.ts with TC09-TC12 test cases SUMMARY: .planning/phases/12.5-mfa-polishing-uat-e2e/12.5-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t cases - ISSUE-004 marked RESOLVED (SecurityTab wired in Plan 01) - TC09-TC12 updated to PASS (wallet E2E tests + interactive verification) - TC27, TC30 updated to PASS (MFA enabled state verified interactively) - TC25-26, TC28-29, TC31 updated from BLOCK to SKIP with reasons - TC15-23 SKIP reasons clarified (multi-device/fresh account required) - Session log updated with final UAT session results - Summary: 16 PASS, 19 SKIP, 1 NOTE — all issues RESOLVED Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 3/3 - Task 1: Run wallet E2E tests (6/6 pass, no changes needed) - Task 2: Interactive Playwright UAT verification (checkpoint approved) - Task 3: Update UAT document with final statuses for all 36 test cases Phase 12.5 complete: 16 PASS, 19 SKIP, 1 NOTE. All 4 issues RESOLVED. SUMMARY: .planning/phases/12.5-mfa-polishing-uat-e2e/12.5-03-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughConsolidates Settings UI into SettingsPage with ARIA tabs (exposing SecurityTab), deletes orphaned Settings.tsx, adds wallet E2E tests (TC09–TC12) with a mock wallet, extends LoginPage page-object for wallet flows, updates styles/design tokens, and marks Phase 12.5 complete in planning/UAT docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Playwright Test
participant Browser as Browser (App)
participant Wallet as Mock Wallet (`@johanneskares/wallet-mock`)
participant Backend as Server (SIWE nonce/verify)
Test->>Browser: installMockWallet() then navigate to /login
Browser->>Wallet: EIP-6963 discovery (request connectors)
Wallet-->>Browser: announce connector (Mock Wallet)
Test->>Browser: click wallet login button
Browser->>Wallet: request connector list / show prompt
Test->>Browser: select "Mock Wallet"
Browser->>Wallet: request signature (SIWE message)
Wallet-->>Browser: signature + address
Browser->>Backend: POST /siwe/nonce -> POST /siwe/verify (verifyMessage)
Backend-->>Browser: verification result (success/error)
Browser-->>Test: redirect to /files OR show error/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labelsrender-preview 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.planning/STATE.md (1)
110-121:⚠️ Potential issue | 🟡 MinorPending todo count mismatch: states 8 but lists 10 items.
Lines 112–121 contain 10 todo items, but line 110 says "8 pending todo(s)". This is a minor documentation inconsistency. Based on learnings,
.planning/STATE.mdmetrics are auto-generated and minor arithmetic inconsistencies are acceptable, so flagging as informational only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/STATE.md around lines 110 - 121, Update the pending-todo count at the top of .planning/STATE.md so the header "8 pending todo(s):" matches the actual list of items (10); specifically change that header to "10 pending todo(s):" or re-run the generator that produces the metric so it auto-syncs with the listed filenames (e.g., `2026-02-07-web-worker-large-file-encryption.md`, `2026-02-10-fix-flaky-post-reload-e2e-tests.md`, etc.).
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.planning/STATE.md:
- Around line 110-121: Update the pending-todo count at the top of
.planning/STATE.md so the header "8 pending todo(s):" matches the actual list of
items (10); specifically change that header to "10 pending todo(s):" or re-run
the generator that produces the metric so it auto-syncs with the listed
filenames (e.g., `2026-02-07-web-worker-large-file-encryption.md`,
`2026-02-10-fix-flaky-post-reload-e2e-tests.md`, etc.).
In `@apps/web/src/routes/SettingsPage.tsx`:
- Around line 58-81: The tablist buttons (ids "tab-linked-methods" and
"tab-security" in SettingsPage.tsx) lack roving tabindex and keyboard handlers;
update the tab implementation so only the active tab has tabIndex={0} and
inactive tabs have tabIndex={-1}, add onKeyDown handler on the tab buttons to
handle ArrowLeft/ArrowRight to move focus (and optionally activate) between
tabs, and Home/End to jump to first/last tab, and use refs (e.g., an array of
tab refs) to call .focus() when moving; keep using activeTab and setActiveTab to
update state when arrows or Home/End activate a tab.
- Around line 84-100: The tab panels currently use both hidden={activeTab !==
'...'} and conditional rendering ({activeTab === '...' && <Component />}), which
is redundant; decide whether to preserve component state or unmount inactive
panels: to preserve state, keep the <LinkedMethods /> and <SecurityTab /> always
rendered inside their respective divs and remove the conditional expressions so
only hidden toggles visibility; to unmount inactive panels, keep the conditional
rendering and remove the hidden={...} attributes on the divs (update the divs
with id="panel-linked-methods" and id="panel-security" accordingly and rely
solely on activeTab checks).
In `@tests/e2e/package.json`:
- Around line 15-21: Update the viem dependency in tests/e2e package.json by
changing the version specifier for "viem" from "^2.44.4" to "^2.46.1"; open the
dependencies block where "viem" is declared (the dependencies array containing
"@johanneskares/wallet-mock", "@noble/ed25519", "@playwright/test",
"@types/node", "dotenv", "typescript", "viem") and modify the "viem" entry, then
run npm install (or yarn) to refresh lockfile and verify tests still pass.
In `@tests/e2e/page-objects/login.page.ts`:
- Around line 150-152: The cancelWalletLogin method clicks the cancel button
without ensuring it's visible, causing flakiness; update async
cancelWalletLogin() to wait for this.walletConnectorCancel.waitFor({ state:
'visible' }) (mirroring clickWalletLogin) before calling
this.walletConnectorCancel.click() so the element is present and visible prior
to interaction.
- Around line 36-59: The wallet selectors are inconsistent and fragile:
walletLoginButton uses data-testid but walletConnectorList,
walletConnectorCancel, walletNoProviders, walletLoginStatus, and walletError use
CSS class selectors; change those to stable data-testid selectors (e.g., replace
'.wallet-connector-list' etc. with matching [data-testid="..."] values), update
the page object getters (walletConnectorList, walletConnectorCancel,
walletNoProviders, walletLoginStatus, walletError) to use those data-testid
attributes, and make sure the app markup provides the corresponding data-testid
attributes so the tests remain stable against styling changes.
In `@tests/e2e/tests/wallet-login.spec.ts`:
- Around line 22-23: Test private key HARDHAT_ACCOUNT_0_KEY is a known Hardhat
test key and should be whitelisted for Gitleaks; to fix, add an inline
suppression comment on the constant (e.g., append "# gitleaks:allow" to the
declaration of HARDHAT_ACCOUNT_0_KEY) or add a .gitleaksignore entry referencing
this file or the exact secret string so the scanner treats it as an allowed
false positive, ensuring the symbol HARDHAT_ACCOUNT_0_KEY remains unchanged and
CI no longer flags it.
- Around line 63-68: The test is using immediate, non-retrying assertions after
UI interactions which can flake; update calls that check the connector list
visibility and contents (e.g., after clickWalletLogin()) to use Playwright's
auto-retrying locator assertions or toPass wrappers. Concretely, expose and use
the page-object locator (walletConnectorListLocator) with await
expect(locator).toBeVisible() before calling
isWalletConnectorListVisible()/getWalletConnectors(), or wrap the existing
isWalletConnectorListVisible() and getWalletConnectors() checks in await
expect(async () => { ... }).toPass({ timeout: 5000 }); apply the same change to
the cancel-flow assertions that use
isWalletConnectorListVisible()/getWalletConnectors() so all visibility/content
checks retry until stable.
- Around line 264-266: Replace the raw locator in the test with a locator
exposed by the LoginPage to preserve page-object encapsulation: add a
locator-returning member (e.g., a getter named walletErrorLocator) to the
LoginPage class that returns the same locator used by
page.locator('.wallet-login-wrapper .login-error'), then update the test to call
loginPage.walletErrorLocator and pass that to expect(...).toBeVisible({ timeout:
10000 }) instead of using page.locator(...) directly; keep the existing
getWalletError() method for retrieving text if still needed.
- Around line 96-114: The third branch in the Promise.race (the
page.waitForTimeout(5000) branch that checks
loginPage.getWalletStatus()/getWalletError()) can short-circuit a slower but
valid redirect from page.waitForURL('**/files', { timeout: 20000 }), so remove
that fixed 5s timeout branch and replace it with a short-poll loop that
repeatedly calls loginPage.getWalletError() and loginPage.getWalletStatus()
(e.g., every 250–500ms) until either page.waitForURL resolves or the 20s overall
timeout elapses; update the Promise.race to only race page.waitForURL(...) and
the polling/error-check promise so slow-but-successful redirects are not masked.
🧹 Nitpick comments (6)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/SettingsPage.tsx`: - Around line 84-100: The tab panels currently use both hidden={activeTab !== '...'} and conditional rendering ({activeTab === '...' && <Component />}), which is redundant; decide whether to preserve component state or unmount inactive panels: to preserve state, keep the <LinkedMethods /> and <SecurityTab /> always rendered inside their respective divs and remove the conditional expressions so only hidden toggles visibility; to unmount inactive panels, keep the conditional rendering and remove the hidden={...} attributes on the divs (update the divs with id="panel-linked-methods" and id="panel-security" accordingly and rely solely on activeTab checks). In `@tests/e2e/page-objects/login.page.ts`: - Around line 36-59: The wallet selectors are inconsistent and fragile: walletLoginButton uses data-testid but walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, and walletError use CSS class selectors; change those to stable data-testid selectors (e.g., replace '.wallet-connector-list' etc. with matching [data-testid="..."] values), update the page object getters (walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, walletError) to use those data-testid attributes, and make sure the app markup provides the corresponding data-testid attributes so the tests remain stable against styling changes. In `@tests/e2e/tests/wallet-login.spec.ts`: - Around line 22-23: Test private key HARDHAT_ACCOUNT_0_KEY is a known Hardhat test key and should be whitelisted for Gitleaks; to fix, add an inline suppression comment on the constant (e.g., append "# gitleaks:allow" to the declaration of HARDHAT_ACCOUNT_0_KEY) or add a .gitleaksignore entry referencing this file or the exact secret string so the scanner treats it as an allowed false positive, ensuring the symbol HARDHAT_ACCOUNT_0_KEY remains unchanged and CI no longer flags it. - Around line 63-68: The test is using immediate, non-retrying assertions after UI interactions which can flake; update calls that check the connector list visibility and contents (e.g., after clickWalletLogin()) to use Playwright's auto-retrying locator assertions or toPass wrappers. Concretely, expose and use the page-object locator (walletConnectorListLocator) with await expect(locator).toBeVisible() before calling isWalletConnectorListVisible()/getWalletConnectors(), or wrap the existing isWalletConnectorListVisible() and getWalletConnectors() checks in await expect(async () => { ... }).toPass({ timeout: 5000 }); apply the same change to the cancel-flow assertions that use isWalletConnectorListVisible()/getWalletConnectors() so all visibility/content checks retry until stable. - Around line 264-266: Replace the raw locator in the test with a locator exposed by the LoginPage to preserve page-object encapsulation: add a locator-returning member (e.g., a getter named walletErrorLocator) to the LoginPage class that returns the same locator used by page.locator('.wallet-login-wrapper .login-error'), then update the test to call loginPage.walletErrorLocator and pass that to expect(...).toBeVisible({ timeout: 10000 }) instead of using page.locator(...) directly; keep the existing getWalletError() method for retrieving text if still needed. - Around line 96-114: The third branch in the Promise.race (the page.waitForTimeout(5000) branch that checks loginPage.getWalletStatus()/getWalletError()) can short-circuit a slower but valid redirect from page.waitForURL('**/files', { timeout: 20000 }), so remove that fixed 5s timeout branch and replace it with a short-poll loop that repeatedly calls loginPage.getWalletError() and loginPage.getWalletStatus() (e.g., every 250–500ms) until either page.waitForURL resolves or the 20s overall timeout elapses; update the Promise.race to only race page.waitForURL(...) and the polling/error-check promise so slow-but-successful redirects are not masked.apps/web/src/routes/SettingsPage.tsx (1)
84-100: Redundanthiddenattribute alongside conditional rendering.Each panel uses both
hidden={activeTab !== '...'}and{activeTab === '...' && <Component />}. Either approach alone prevents the content from being visible/accessible. Using both means the DOM node is present (withhidden) but the children are unmounted — thehiddenattribute is effectively unused.Pick one strategy. If you want to preserve state across tab switches (e.g., form inputs), remove conditional rendering and rely on
hiddenalone. If you prefer unmounting inactive panels, removehidden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/SettingsPage.tsx` around lines 84 - 100, The tab panels currently use both hidden={activeTab !== '...'} and conditional rendering ({activeTab === '...' && <Component />}), which is redundant; decide whether to preserve component state or unmount inactive panels: to preserve state, keep the <LinkedMethods /> and <SecurityTab /> always rendered inside their respective divs and remove the conditional expressions so only hidden toggles visibility; to unmount inactive panels, keep the conditional rendering and remove the hidden={...} attributes on the divs (update the divs with id="panel-linked-methods" and id="panel-security" accordingly and rely solely on activeTab checks).tests/e2e/page-objects/login.page.ts (1)
36-59: Selector strategy is inconsistent:data-testidfor wallet button, CSS classes for the rest.
walletLoginButtonuses[data-testid="wallet-login-button"]while all other wallet selectors rely on CSS class names (.wallet-connector-list,.wallet-connector-cancel, etc.). CSS class selectors are more brittle—they break on styling refactors. Consider usingdata-testidattributes consistently for all wallet UI elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/login.page.ts` around lines 36 - 59, The wallet selectors are inconsistent and fragile: walletLoginButton uses data-testid but walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, and walletError use CSS class selectors; change those to stable data-testid selectors (e.g., replace '.wallet-connector-list' etc. with matching [data-testid="..."] values), update the page object getters (walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, walletError) to use those data-testid attributes, and make sure the app markup provides the corresponding data-testid attributes so the tests remain stable against styling changes.tests/e2e/tests/wallet-login.spec.ts (4)
22-23: Gitleaks false positive — Hardhat account#0is a well-known test key.This is the default Hardhat/Anvil account
#0private key, publicly documented and deterministic. It holds no real funds and is standard practice for E2E testing. Consider adding an inline# gitleaks:allowcomment or a.gitleaksignoreentry to suppress future CI noise.Suppress Gitleaks alert
// Hardhat account `#0` private key (well-known, deterministic, no real funds) const HARDHAT_ACCOUNT_0_KEY = - '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' as const; + '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' as const; // gitleaks:allow -- Hardhat account `#0`, public test key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/wallet-login.spec.ts` around lines 22 - 23, Test private key HARDHAT_ACCOUNT_0_KEY is a known Hardhat test key and should be whitelisted for Gitleaks; to fix, add an inline suppression comment on the constant (e.g., append "# gitleaks:allow" to the declaration of HARDHAT_ACCOUNT_0_KEY) or add a .gitleaksignore entry referencing this file or the exact secret string so the scanner treats it as an allowed false positive, ensuring the symbol HARDHAT_ACCOUNT_0_KEY remains unchanged and CI no longer flags it.
63-68: Bare assertions afterclickWalletLogin()may be flaky.Lines 64 and 166–176 perform immediate
expect(…).toBe(true/false)checks right after UI interactions (click, cancel) without any retry/polling. If the connector list renders asynchronously or an animation is involved, these will intermittently fail. Wrap them inexpect(…).toPass()or use Playwright's auto-retryingexpect(locator).toBeVisible()instead.Example: use auto-retrying locator assertions
For instance, instead of:
- expect(await loginPage.isWalletConnectorListVisible()).toBe(true);Expose the underlying locator from the page object and use:
await expect(loginPage.walletConnectorListLocator).toBeVisible();Or at minimum wrap in
toPass:await expect(async () => { expect(await loginPage.isWalletConnectorListVisible()).toBe(true); }).toPass({ timeout: 5000 });The same applies to the cancel-flow assertions on lines 172–176 and line 219.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/wallet-login.spec.ts` around lines 63 - 68, The test is using immediate, non-retrying assertions after UI interactions which can flake; update calls that check the connector list visibility and contents (e.g., after clickWalletLogin()) to use Playwright's auto-retrying locator assertions or toPass wrappers. Concretely, expose and use the page-object locator (walletConnectorListLocator) with await expect(locator).toBeVisible() before calling isWalletConnectorListVisible()/getWalletConnectors(), or wrap the existing isWalletConnectorListVisible() and getWalletConnectors() checks in await expect(async () => { ... }).toPass({ timeout: 5000 }); apply the same change to the cancel-flow assertions that use isWalletConnectorListVisible()/getWalletConnectors() so all visibility/content checks retry until stable.
264-266: Raw locator breaks page-object encapsulation.
page.locator('.wallet-login-wrapper .login-error')duplicates knowledge of the DOM structure that should live inLoginPage. ThegetWalletError()method (used on line 269) already abstracts this. Consider adding a locator-returning method toLoginPageso this line can use auto-retryingexpect(locator).toBeVisible()via the page object.Proposed approach
In
LoginPage, expose:get walletErrorLocator() { return this.page.locator('.wallet-login-wrapper .login-error'); }Then in the test:
- const errorLocator = page.locator('.wallet-login-wrapper .login-error'); - await expect(errorLocator).toBeVisible({ timeout: 10000 }); + await expect(loginPage.walletErrorLocator).toBeVisible({ timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/wallet-login.spec.ts` around lines 264 - 266, Replace the raw locator in the test with a locator exposed by the LoginPage to preserve page-object encapsulation: add a locator-returning member (e.g., a getter named walletErrorLocator) to the LoginPage class that returns the same locator used by page.locator('.wallet-login-wrapper .login-error'), then update the test to call loginPage.walletErrorLocator and pass that to expect(...).toBeVisible({ timeout: 10000 }) instead of using page.locator(...) directly; keep the existing getWalletError() method for retrieving text if still needed.
96-114:Promise.racewithwaitForTimeout(5000)may short-circuit valid flows.The third branch resolves after a fixed 5 s wait. If the redirect (branch 1) takes between 5–20 s, the timeout branch wins the race and the test reports
"in-progress"or"timeout"instead of"redirected". Since the test body already accepts non-redirect outcomes gracefully, this won't fail — but it may mask real successes in environments where the backend is available but slow.Consider increasing the fixed wait or removing the third branch entirely and relying on the 20 s
waitForURLtimeout plus a shortergetWalletErrorpoll to cover both outcomes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/wallet-login.spec.ts` around lines 96 - 114, The third branch in the Promise.race (the page.waitForTimeout(5000) branch that checks loginPage.getWalletStatus()/getWalletError()) can short-circuit a slower but valid redirect from page.waitForURL('**/files', { timeout: 20000 }), so remove that fixed 5s timeout branch and replace it with a short-poll loop that repeatedly calls loginPage.getWalletError() and loginPage.getWalletStatus() (e.g., every 250–500ms) until either page.waitForURL resolves or the 20s overall timeout elapses; update the Promise.race to only race page.waitForURL(...) and the polling/error-check promise so slow-but-successful redirects are not masked.
- Add 19 wallet login CSS classes matching terminal aesthetic - Change MFA enrollment banner to amber accent (rgb(245 158 11)) - Update SecurityTab enable-btn to font-size-sm with larger padding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dMethods - Change wallet icon from Greek Xi to "W" for universal legibility - Add visible "// last method" hint next to disabled unlink button - Add CSS for linked-methods-unlink-hint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 (+ 1 pending human verification) - Add WalletLoginButton CSS and fix MFA banner styling - Fix wallet icon and unlink disabled state in LinkedMethods SUMMARY: .planning/quick/016-refine-wallet-and-mfa-ui-elements/016-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Quick task completed — wallet login fully styled, MFA banner amber accent, security enable button sized up, wallet icon and unlink hint improved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update Pencil frames to match CSS implementation (5 discrepancies fixed) - Add warning/amber color tokens, text-dim, error colors to DESIGN.md - Add Wallet Button, MFA Banner, Connector Picker, Linked Method patterns - Add 4 design decisions from Phase 12.5 refinement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@designs/DESIGN.md`:
- Around line 13-25: The table defines two different hex values for the same CSS
variable (--color-text-muted) which is ambiguous; pick which hex should remain
as --color-text-muted and rename the other to a distinct variable (e.g.,
--color-text-dim, --color-label-muted or --color-text-muted-variant) and update
its Usage entry accordingly so each hex maps to a unique CSS variable; ensure
references to --color-text-muted, `#006644`, and `#8b9a8f` in the DESIGN.md table
are adjusted to reflect the chosen mapping and new variable name.
Comment on lines
+13
to
+25
| | Hex Code | Opacity | Usage | CSS Variable | | ||
| | --------- | ------- | -------------------------- | ---------------------- | | ||
| | `#000000` | 100% | Background, surfaces | `--color-background` | | ||
| | `#00D084` | 100% | Primary accent, borders | `--color-primary` | | ||
| | `#00D084` | 40% | Glow effects | `--color-glow` | | ||
| | `#006644` | 100% | Secondary text, muted | `--color-text-muted` | | ||
| | `#003322` | 100% | Row separators, dividers | `--color-border-muted` | | ||
| | `#F59E0B` | 100% | Warning accent, MFA banner | `--color-warning` | | ||
| | `#F59E0B` | 8% | Warning background tint | — | | ||
| | `#F59E0B` | 25% | Warning border | — | | ||
| | `#EF4444` | 100% | Error, danger actions | `--color-error` | | ||
| | `#4a5a4e` | 100% | Dim text, dismiss buttons | `--color-text-dim` | | ||
| | `#8b9a8f` | 100% | Muted labels | `--color-text-muted` | |
There was a problem hiding this comment.
Duplicate --color-text-muted mapping with different hex values.
Line 18 maps #006644 to --color-text-muted ("Secondary text, muted") and Line 25 maps #8b9a8f to the same --color-text-muted ("Muted labels"). Two different hex values for the same CSS variable name will cause confusion when referencing this design system doc for implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@designs/DESIGN.md` around lines 13 - 25, The table defines two different hex
values for the same CSS variable (--color-text-muted) which is ambiguous; pick
which hex should remain as --color-text-muted and rename the other to a distinct
variable (e.g., --color-text-dim, --color-label-muted or
--color-text-muted-variant) and update its Usage entry accordingly so each hex
maps to a unique CSS variable; ensure references to --color-text-muted, `#006644`,
and `#8b9a8f` in the DESIGN.md table are adjusted to reflect the chosen mapping
and new variable name.
- Add roving tabindex and arrow key navigation to settings tabs (WAI-ARIA) - Update viem to ^2.46.1 in e2e devDependencies - Add waitFor visibility check before click in cancelWalletLogin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/page-objects/login.page.ts`:
- Around line 126-136: The getWalletConnectors function loops over options with
count/nth/textContent causing sequential awaits; replace that logic by using
Playwright's options.allTextContents() to fetch all texts at once, then
map/clean each string (remove surrounding brackets and trim) before returning;
update references to walletConnectorList.waitFor(...) and the options locator
('.wallet-connector-option') within getWalletConnectors to use allTextContents()
and perform the same regex replace on each item.
- Around line 36-59: Replace the brittle CSS-class locators with stable
data-testid locators: update the component to add data-testid attributes for the
wallet elements and then change the page-object getters walletConnectorList,
walletConnectorCancel, walletNoProviders, walletLoginStatus, walletError to use
this.page.locator('[data-testid="..."]') instead of class selectors; also add a
new getter walletConnectorOption (e.g.,
this.page.locator('[data-testid="wallet-connector-option"]')) and update the
inline usages of '.wallet-connector-option' to use this.walletConnectorOption so
all wallet selectors consistently rely on data-testid.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/login.page.ts`: - Around line 126-136: The getWalletConnectors function loops over options with count/nth/textContent causing sequential awaits; replace that logic by using Playwright's options.allTextContents() to fetch all texts at once, then map/clean each string (remove surrounding brackets and trim) before returning; update references to walletConnectorList.waitFor(...) and the options locator ('.wallet-connector-option') within getWalletConnectors to use allTextContents() and perform the same regex replace on each item. - Around line 36-59: Replace the brittle CSS-class locators with stable data-testid locators: update the component to add data-testid attributes for the wallet elements and then change the page-object getters walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, walletError to use this.page.locator('[data-testid="..."]') instead of class selectors; also add a new getter walletConnectorOption (e.g., this.page.locator('[data-testid="wallet-connector-option"]')) and update the inline usages of '.wallet-connector-option' to use this.walletConnectorOption so all wallet selectors consistently rely on data-testid.tests/e2e/page-objects/login.page.ts (2)
126-136:getWalletConnectorscan be simplified withallTextContents().The manual loop over
count/nth(i)/textContent()can be replaced with Playwright's built-inallTextContents(), which returns all matching elements' text in one call. This also avoids the implicit sequential await per element.Simplified implementation
async getWalletConnectors(): Promise<string[]> { await this.walletConnectorList.waitFor({ state: 'visible', timeout: 5000 }); - const options = this.page.locator('.wallet-connector-option'); - const count = await options.count(); - const names: string[] = []; - for (let i = 0; i < count; i++) { - const text = await options.nth(i).textContent(); - if (text) names.push(text.replace(/^\[|\]$/g, '').trim()); - } - return names; + const texts = await this.page.locator('.wallet-connector-option').allTextContents(); + return texts + .map(t => t.replace(/^\[|\]$/g, '').trim()) + .filter(Boolean); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/login.page.ts` around lines 126 - 136, The getWalletConnectors function loops over options with count/nth/textContent causing sequential awaits; replace that logic by using Playwright's options.allTextContents() to fetch all texts at once, then map/clean each string (remove surrounding brackets and trim) before returning; update references to walletConnectorList.waitFor(...) and the options locator ('.wallet-connector-option') within getWalletConnectors to use allTextContents() and perform the same regex replace on each item.
36-59: Inconsistent selector strategy: CSS classes vs.data-testidattributes.The existing email/Google selectors and
walletLoginButtonall use[data-testid="..."], but the remaining wallet selectors rely on plain CSS classes (.wallet-connector-list,.wallet-connector-cancel, etc.). CSS classes are styling concerns and can be renamed or removed during refactors without breaking production code — but they will silently break these tests. Preferdata-testidattributes for all test-facing selectors to keep the test suite resilient.Also,
.wallet-connector-optionis used inline at line 128 and line 143 but isn't declared as a getter alongside the other selectors, further splitting the locator definitions.Suggested approach — add data-testid attributes in the component and update selectors here
private get walletConnectorList() { - return this.page.locator('.wallet-connector-list'); + return this.page.locator('[data-testid="wallet-connector-list"]'); } private get walletConnectorCancel() { - return this.page.locator('.wallet-connector-cancel'); + return this.page.locator('[data-testid="wallet-connector-cancel"]'); } private get walletNoProviders() { - return this.page.locator('.wallet-no-providers'); + return this.page.locator('[data-testid="wallet-no-providers"]'); } private get walletLoginStatus() { - return this.page.locator('.wallet-login-status'); + return this.page.locator('[data-testid="wallet-login-status"]'); } private get walletError() { - return this.page.locator('.wallet-login-wrapper .login-error'); + return this.page.locator('[data-testid="wallet-login-error"]'); } + + private get walletConnectorOption() { + return this.page.locator('[data-testid="wallet-connector-option"]'); + }Then use
this.walletConnectorOptionat lines 128 and 143 instead of the inline locator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/login.page.ts` around lines 36 - 59, Replace the brittle CSS-class locators with stable data-testid locators: update the component to add data-testid attributes for the wallet elements and then change the page-object getters walletConnectorList, walletConnectorCancel, walletNoProviders, walletLoginStatus, walletError to use this.page.locator('[data-testid="..."]') instead of class selectors; also add a new getter walletConnectorOption (e.g., this.page.locator('[data-testid="wallet-connector-option"]')) and update the inline usages of '.wallet-connector-option' to use this.walletConnectorOption so all wallet selectors consistently rely on data-testid.
The field was renamed in Phase 12.3 but the E2E helper was not updated, leaving vaultKeypair null and causing folder creation to throw before closing the dialog. Also close create-folder dialog in finally block so it always dismisses even on error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
This was referenced Feb 25, 2026
Closed
5 tasks
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@johanneskares/wallet-mockEIP-6963 mock provider (6 tests covering TC09-TC12)Test plan
pnpm --filter e2e test -- tests/wallet-login.spec.ts)pnpm --filter web build)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style / Design
Documentation