test: add Playwright E2E test suite with mock mode and CI#16
Open
thepastaclaw wants to merge 12 commits intoPastaPastaPasta:mainfrom
Open
test: add Playwright E2E test suite with mock mode and CI#16thepastaclaw wants to merge 12 commits intoPastaPastaPasta:mainfrom
thepastaclaw wants to merge 12 commits intoPastaPastaPasta:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds Playwright end-to-end testing (deterministic mock mode and optional live testnet), Playwright and Vitest configs, CI e2e job and npm scripts, .gitignore updates, new e2e specs, exported mock constants/types, and application mock-mode logic gated by ChangesCI & Workflow
NPM Scripts & Dependencies
Playwright & Vitest Configuration
E2E Test Suites
Application Mock Mode
Docs & VCS
Sequence Diagram(s)sequenceDiagram
rect rgba(63,81,181,0.5)
participant CI as CI Runner
end
rect rgba(76,175,80,0.5)
participant Server as Dev Server
end
rect rgba(255,193,7,0.5)
participant Browser as Playwright Browser
end
rect rgba(156,39,176,0.5)
participant App as Application (mock engine)
end
CI->>Server: start webServer (npm run dev) using HOST/PORT
CI->>Browser: launch Playwright, navigate to BASE_URL?e2e=mock
Browser->>App: page loads, checks isE2EMockMode()
Browser->>App: trigger flows (create/top-up/manage/dpns)
App-->>Browser: staged progress events (delays, __e2eMockAdvance)
Browser-->>CI: assertions -> report artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 46 minutes and 19 seconds.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: 6
🧹 Nitpick comments (2)
src/main.ts (1)
599-606: Mock key validation accepts only the exact hardcoded WIF strings.The mock WIFs (
cMockDpnsPrivateKeyWif,cMockManagePrivateKeyWif) are hardcoded on lines 110-111 here and duplicated on lines 5-6 ofe2e/deterministic.spec.ts. If either side changes independently, the tests will silently fail with an unhelpful "Mock mode: use the configured test private key" error.Consider exporting these constants from a shared location or at minimum adding a code comment cross-referencing the test file.
Also applies to: 846-853
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 599 - 606, The mock WIF validation currently compares privateKeyWif against hardcoded constants (cMockDpnsPrivateKeyWif, cMockManagePrivateKeyWif) duplicated in tests, which can cause silent test breakage; refactor by moving those constants into a shared exported constant (e.g., export MOCK_DPNS_WIF and MOCK_MANAGE_WIF from a new/shared module) and import them into src/main.ts (where isE2EMockMode() check lives) and into e2e/deterministic.spec.ts, updating the comparisons in the dpns mock validation block (and the analogous block around setManageKeyValidationError) to use the shared constants; if you cannot refactor now, add a clear code comment in both places referencing e2e/deterministic.spec.ts and the source constant names to ensure they stay in sync.e2e/deterministic.spec.ts (1)
3-6: Mock constants duplicated fromsrc/main.ts.These values must stay in sync with
E2E_MOCK_IDENTITY_ID,E2E_MOCK_DPNS_WIF, andE2E_MOCK_MANAGE_WIFinsrc/main.ts(lines 109-111). A mismatch would cause silent test failures. Consider extracting them to a shared constants file importable by both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/deterministic.spec.ts` around lines 3 - 6, The test defines MOCK_QUERY, MOCK_IDENTITY_ID, MOCK_DPNS_WIF, and MOCK_MANAGE_WIF which duplicate E2E_MOCK_IDENTITY_ID, E2E_MOCK_DPNS_WIF, and E2E_MOCK_MANAGE_WIF in the main code and risk drifting out of sync; refactor by extracting these values into a single shared constants module (e.g., export the constants from a new e2e/constants or test/constants file) and update deterministic.spec.ts to import MOCK_QUERY, MOCK_IDENTITY_ID, MOCK_DPNS_WIF, and MOCK_MANAGE_WIF from that shared module (or import the existing E2E_MOCK_* exports if appropriate) so both the test and the application use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-41: The e2e job currently never uploads Playwright artifacts;
update the e2e job (job id "e2e") to upload Playwright artifacts on failure by
adding an artifacts upload step that runs conditionally (if: failure() or
always()) and points to the Playwright output directories such as
playwright-report/ and test-results/ (include traces, screenshots and videos if
present), so add a step after the test step that uses actions/upload-artifact@v4
(or equivalent) to upload those directories; ensure the step references the same
job name "e2e" and the directories "playwright-report/" and "test-results/" so
CI failures provide traces for debugging.
In `@e2e/deterministic.spec.ts`:
- Around line 22-25: The transient assertion for "Creating your identity" is
flaky because __e2eMockAdvance() advances the mock bridge through multiple
states quickly; remove the short-lived assertion await
expect(page.getByText('Creating your identity')).toBeVisible() and only assert
the stable final state ("Save your keys") and identity ID, or alternatively
increase the delay in the mock flow where setIdentityRegistered is called (the
mock bridge progression invoked by __e2eMockAdvance()) so the
registering_identity state is visible long enough for Playwright's retrier to
observe it.
- Around line 56-58: The test asserts a transient "Processing top-up" text which
can disappear before Playwright sees it; remove the flaky assertion against
page.getByText('Processing top-up') and instead assert the stable final state by
awaiting visibility of the final message (e.g., page.getByText('Top-up
complete!')) after the existing await
expect(page.locator('.deposit-headline')).toBeVisible(); ensure you update the
assertion sequence in deterministic.spec.ts to check for the final "Top-up
complete!" text rather than the transient "Processing top-up".
In `@README.md`:
- Around line 70-72: The `(CRITICAL/HIGH)` annotation on
PW_LIVE_DPNS_PRIVATE_KEY_WIF is ambiguous; update the README line for
PW_LIVE_DPNS_PRIVATE_KEY_WIF to replace that parenthetical with a clearer phrase
like "DPNS authentication key (Dash Platform key purpose/priority level)" or a
short explanatory note and/or add a link to the Dash Platform docs that describe
key priorities so readers understand the risk/priority semantics; ensure the
wording is mirrored for the other identity-related entries if applicable
(PW_LIVE_MANAGE_IDENTITY_ID, PW_LIVE_MANAGE_PRIVATE_KEY_WIF).
- Around line 56-63: The README shows setting WIF private keys inline (e.g.,
PW_LIVE_DPNS_PRIVATE_KEY_WIF, PW_LIVE_MANAGE_PRIVATE_KEY_WIF) which will record
secrets in shell history; change the snippet to warn users and provide safer
alternatives: add a brief callout warning about history exposure and recommend
exporting variables from a protected env file or using a secure prompt/read -s,
sourcing from a file with strict permissions, or using a credential manager, and
show the env var names (PW_E2E_LIVE, PW_LIVE_TOPUP_IDENTITY_ID,
PW_LIVE_DPNS_IDENTITY_ID, PW_LIVE_DPNS_PRIVATE_KEY_WIF,
PW_LIVE_MANAGE_IDENTITY_ID, PW_LIVE_MANAGE_PRIVATE_KEY_WIF) so readers know
which variables to set securely.
In `@src/main.ts`:
- Around line 109-193: The file includes E2E mock constants, types and helper
functions (E2EMockWindow, isE2EMockMode, createE2EMockUtxo,
createE2EMockIdentityKeys, createE2EMockDpnsAvailability,
createE2EMockDpnsResults, clearE2EMockAdvanceHook, waitForE2EMockAdvance and the
E2E_* constants) that are shipped into production and can be enabled via a URL
param; move all test-only code into a separate dev-only module (e.g.,
src/e2e-mocks.ts) and the E2EMockWindow type into src/types.ts, then guard any
imports/usage with a build-time flag (import.meta.env.DEV or a custom VITE_E2E)
so Vite can tree-shake the mock module from production (or use dynamic import
inside an if (import.meta.env.DEV || import.meta.env.VITE_E2E) block), and
ensure isE2EMockMode becomes local to that dev-only module so production builds
no longer include the mock helpers or constants.
---
Nitpick comments:
In `@e2e/deterministic.spec.ts`:
- Around line 3-6: The test defines MOCK_QUERY, MOCK_IDENTITY_ID, MOCK_DPNS_WIF,
and MOCK_MANAGE_WIF which duplicate E2E_MOCK_IDENTITY_ID, E2E_MOCK_DPNS_WIF, and
E2E_MOCK_MANAGE_WIF in the main code and risk drifting out of sync; refactor by
extracting these values into a single shared constants module (e.g., export the
constants from a new e2e/constants or test/constants file) and update
deterministic.spec.ts to import MOCK_QUERY, MOCK_IDENTITY_ID, MOCK_DPNS_WIF, and
MOCK_MANAGE_WIF from that shared module (or import the existing E2E_MOCK_*
exports if appropriate) so both the test and the application use the same source
of truth.
In `@src/main.ts`:
- Around line 599-606: The mock WIF validation currently compares privateKeyWif
against hardcoded constants (cMockDpnsPrivateKeyWif, cMockManagePrivateKeyWif)
duplicated in tests, which can cause silent test breakage; refactor by moving
those constants into a shared exported constant (e.g., export MOCK_DPNS_WIF and
MOCK_MANAGE_WIF from a new/shared module) and import them into src/main.ts
(where isE2EMockMode() check lives) and into e2e/deterministic.spec.ts, updating
the comparisons in the dpns mock validation block (and the analogous block
around setManageKeyValidationError) to use the shared constants; if you cannot
refactor now, add a clear code comment in both places referencing
e2e/deterministic.spec.ts and the source constant names to ensure they stay in
sync.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main.ts (3)
123-131: Inlineimport('./types.js').UTXOis inconsistent with the top-levelimport typeblock.Replace with the top-level
UTXOimport already suggested above.-function createE2EMockUtxo(): import('./types.js').UTXO { +function createE2EMockUtxo(): UTXO {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 123 - 131, The inline type annotation "import('./types.js').UTXO" in the createE2EMockUtxo function should be replaced with the top-level imported type name UTXO (use the existing `import type { UTXO } from './types.js'`), so update the function signature for createE2EMockUtxo to return UTXO and remove the inline import reference to keep type imports consistent with the file's import type block.
1062-1083: AddclearE2EMockAdvanceHook()beforewaitForE2EMockAdvance()in the top-up mock for consistency with the bridge mock (line 1198).Without it, a stale hook from a cancelled bridge flow would be silently overwritten rather than explicitly cleared, making the intent less obvious.
if (isE2EMockMode()) { const network = getNetwork(state.network); const assetLockKeyPair = generateKeyPair(); const depositAddress = publicKeyToAddress(assetLockKeyPair.publicKey, network); + clearE2EMockAdvanceHook(); updateState(setStep(state, 'generating_keys'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1062 - 1083, In the isE2EMockMode() top-up mock branch, call clearE2EMockAdvanceHook() immediately before invoking waitForE2EMockAdvance() so any stale advance hook from a cancelled bridge flow is explicitly cleared; locate the block using isE2EMockMode(), updateState(...) calls and replace the current sequence where waitForE2EMockAdvance() is awaited to first call clearE2EMockAdvanceHook(), then await waitForE2EMockAdvance(), leaving the rest of the mock sequence (setUtxoDetected, setTransactionSigned, setTransactionBroadcast, setInstantLockReceived, setTopUpComplete) unchanged.
93-102: Merge the twoimport typestatements from'./types.js'and addUTXOto the top-level import (see line 123).♻️ Proposed fix
-import type { - KeyType, - KeyPurpose, - SecurityLevel, - ManageNewKeyConfig, - DpnsUsernameEntry, - DpnsRegistrationResult, - IdentityPublicKeyInfo, -} from './types.js'; -import type { BridgeState } from './types.js'; +import type { + KeyType, + KeyPurpose, + SecurityLevel, + ManageNewKeyConfig, + DpnsUsernameEntry, + DpnsRegistrationResult, + IdentityPublicKeyInfo, + BridgeState, + UTXO, +} from './types.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 93 - 102, Two separate `import type` blocks from './types.js' should be combined into a single top-level import and must include the missing UTXO type; update the import to import KeyType, KeyPurpose, SecurityLevel, ManageNewKeyConfig, DpnsUsernameEntry, DpnsRegistrationResult, IdentityPublicKeyInfo, BridgeState and UTXO from './types.js' in one statement so references like KeyType, BridgeState and UTXO resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.ts`:
- Around line 185-193: The waitForE2EMockAdvance function currently waits
indefinitely; add an internal timeout (e.g., 5s or configurable) that rejects
with a descriptive Error if __e2eMockAdvance is not called in time, and ensure
you clear the timer and remove/undefine mockWindow.__e2eMockAdvance on both
resolve and reject to avoid leaks; locate function waitForE2EMockAdvance and
implement a setTimeout that calls reject(new Error("Timed out waiting for E2E
mock advance")) and clearTimeout in the success callback.
---
Duplicate comments:
In `@src/main.ts`:
- Around line 109-193: The PR accidentally includes E2E mock utilities
(E2E_MOCK_IDENTITY_ID, E2E_MOCK_DPNS_WIF, isE2EMockMode, createE2EMockUtxo,
createE2EMockIdentityKeys, createE2EMockDpnsAvailability,
createE2EMockDpnsResults, waitForE2EMockAdvance, clearE2EMockAdvanceHook) in
production bundle; move all of these into a separate test-only module (e.g.,
src/e2e/mocks.ts) and remove their exports/usage from src/main.ts, or wrap usage
behind a compile-time flag so the bundler can tree-shake them out (e.g., use
process.env.NODE_ENV or a dedicated BUILD_E2E flag and conditional import),
ensuring main.ts only imports mocks during test/e2e builds so they are not
shipped in production.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 123-131: The inline type annotation "import('./types.js').UTXO" in
the createE2EMockUtxo function should be replaced with the top-level imported
type name UTXO (use the existing `import type { UTXO } from './types.js'`), so
update the function signature for createE2EMockUtxo to return UTXO and remove
the inline import reference to keep type imports consistent with the file's
import type block.
- Around line 1062-1083: In the isE2EMockMode() top-up mock branch, call
clearE2EMockAdvanceHook() immediately before invoking waitForE2EMockAdvance() so
any stale advance hook from a cancelled bridge flow is explicitly cleared;
locate the block using isE2EMockMode(), updateState(...) calls and replace the
current sequence where waitForE2EMockAdvance() is awaited to first call
clearE2EMockAdvanceHook(), then await waitForE2EMockAdvance(), leaving the rest
of the mock sequence (setUtxoDetected, setTransactionSigned,
setTransactionBroadcast, setInstantLockReceived, setTopUpComplete) unchanged.
- Around line 93-102: Two separate `import type` blocks from './types.js' should
be combined into a single top-level import and must include the missing UTXO
type; update the import to import KeyType, KeyPurpose, SecurityLevel,
ManageNewKeyConfig, DpnsUsernameEntry, DpnsRegistrationResult,
IdentityPublicKeyInfo, BridgeState and UTXO from './types.js' in one statement
so references like KeyType, BridgeState and UTXO resolve correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main.ts (1)
1064-1085:clearE2EMockAdvanceHook()missing fromstartTopUpcatch block, but present unconditionally instartBridgecatch.Two inconsistencies:
startBridgecallsclearE2EMockAdvanceHook()at the start of its mock path (line 1200) and in its catch block (line 1328), butstartTopUpdoes neither — ifwaitForE2EMockAdvance(line 1073) is pending when an error occurs, the dangling hook is never cleaned up.Line 1328 calls
clearE2EMockAdvanceHook()unconditionally (even for non-mock errors). This is harmless but untidy — it should be inside anisE2EMockMode()guard or, better yet, handled by the build-time gating suggested in the prior review.♻️ Add cleanup to startTopUp catch block for symmetry
} catch (error) { + clearE2EMockAdvanceHook(); console.error('Top-up error:', error); updateState(setError(state, error instanceof Error ? error : new Error(String(error)))); }Also applies to: 1195-1216, 1328-1328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1064 - 1085, The startTopUp flow can leave a dangling E2E mock hook if waitForE2EMockAdvance throws, so update startTopUp's catch block to call clearE2EMockAdvanceHook() (only when isE2EMockMode() is true) before rethrowing/handling the error; likewise, in startBridge's catch replace the unconditional clearE2EMockAdvanceHook() with a guarded call wrapped by isE2EMockMode() or consolidate cleanup into a small helper invoked only in mock mode, referencing startTopUp, startBridge, waitForE2EMockAdvance, clearE2EMockAdvanceHook, and isE2EMockMode to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.ts`:
- Around line 155-170: The mock uses entry.label.toLowerCase() for contest
checks which diverges from production behavior; update
createE2EMockDpnsAvailability to use the homograph-normalized label
(entry.normalizedLabel) when computing contest/availability. Specifically, in
createE2EMockDpnsAvailability replace the use of lower =
entry.label.toLowerCase() with the normalized label (or
normalizedLabel.toLowerCase() if you want a case-fold) and pass that value into
isContestedUsername (and use it when deriving isAvailable/status) so the mock
mirrors production's normalized checks.
---
Duplicate comments:
In `@src/main.ts`:
- Around line 110-195: The E2E mock helpers (isE2EMockMode, createE2EMockUtxo,
createE2EMockIdentityKeys, createE2EMockDpnsAvailability,
createE2EMockDpnsResults, clearE2EMockAdvanceHook, waitForE2EMockAdvance) are
being shipped into production and the waitForE2EMockAdvance has no timeout;
guard their inclusion with a build-time check (use import.meta.env.PROD or
equivalent) so these functions are only imported/defined in non-production
builds, and modify waitForE2EMockAdvance to race the resolve callback against a
setTimeout that rejects after a configurable timeout (ensure you clear the
timeout and remove the __e2eMockAdvance hook on both resolve and reject by
calling/using clearE2EMockAdvanceHook) so the promise never hangs.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 1064-1085: The startTopUp flow can leave a dangling E2E mock hook
if waitForE2EMockAdvance throws, so update startTopUp's catch block to call
clearE2EMockAdvanceHook() (only when isE2EMockMode() is true) before
rethrowing/handling the error; likewise, in startBridge's catch replace the
unconditional clearE2EMockAdvanceHook() with a guarded call wrapped by
isE2EMockMode() or consolidate cleanup into a small helper invoked only in mock
mode, referencing startTopUp, startBridge, waitForE2EMockAdvance,
clearE2EMockAdvanceHook, and isE2EMockMode to locate the changes.
Contributor
Author
|
Addressed the latest CodeRabbit feedback in 27e0b6f:
Validation run locally:
|
Contributor
Author
|
Triage follow-up on the mention: all currently actionable CodeRabbit comments appear addressed in commit 27e0b6f and acknowledged in-thread. I don’t see any remaining unresolved changes requested from the mention context, so no additional code changes are needed from this ping. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main.ts (1)
1034-1052:manageMoreBtnhandler has no mock path — calls real network in mock mode.After
startManageUpdatecompletes in mock mode the UI shows an "Update Complete!" page with a "Manage More" button. Clicking it invokesgetIdentityPublicKeys(E2E_MOCK_IDENTITY_ID, network)against real testnet. The mock identity'1111…1'doesn't exist, so the call fails and surfaces an error. The deterministic tests don't exercise this button, so CI won't catch it, but it breaks manual mock-mode exploration.♻️ Suggested fix
if (targetId) { try { + if (isE2EMockMode()) { + updateState(setManageIdentityFetched(refreshedState, createE2EMockIdentityKeys())); + return; + } const keys = await getIdentityPublicKeys(targetId, refreshedState.network);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1034 - 1052, The manageMoreBtn click handler calls getIdentityPublicKeys for the recovered refreshedState and so triggers real network calls in mock mode; modify the handler to detect mock-mode (e.g., check refreshedState.network or an existing isMock flag used elsewhere) and in that case avoid calling getIdentityPublicKeys — instead populate the state with the deterministic/mock identity keys (or a noop success) by calling setManageIdentityFetched with the mock keys (e.g., for E2E_MOCK_IDENTITY_ID) or skip to reset state; preserve the current error handling path for non-mock networks so only real networks invoke getIdentityPublicKeys, and still call updateState(setManageIdentityFetchError(...)) on real failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/deterministic.spec.ts`:
- Line 88: Replace the flaky Playwright .check() call on the manage-disable-key
checkbox with a forced click to avoid actionability timeouts: locate the element
used in deterministic.spec.ts (the expression
page.locator('.manage-disable-key-checkbox').first()) and call .click({ force:
true }) on that locator instead of .check() so the test is stable in headless CI
and matches the existing pattern used for `#dpns-contested-checkbox`.
In `@README.md`:
- Around line 56-72: The .env.playwright-live file containing private keys must
be excluded from version control; update the repository .gitignore by adding
either the exact entry ".env.playwright-live" or a broader pattern like ".env.*"
so any env files (including .env.playwright-live) are ignored; ensure the change
references the env filename used in the README (".env.playwright-live") so
accidental git adds won't commit secrets.
---
Duplicate comments:
In `@src/main.ts`:
- Around line 116-118: Move the E2EMockWindow type out of its current location
and into the central types file where other interfaces live: create (or add) an
exported type E2EMockWindow = Window & { __e2eMockAdvance?: () => void } in
src/types.ts, remove the local declaration from main.ts, and update any files
that reference E2EMockWindow (including the code that uses E2E_MOCK_* constants)
to import the type from the shared types module; ensure the type is exported so
consumers compile without changes.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 1034-1052: The manageMoreBtn click handler calls
getIdentityPublicKeys for the recovered refreshedState and so triggers real
network calls in mock mode; modify the handler to detect mock-mode (e.g., check
refreshedState.network or an existing isMock flag used elsewhere) and in that
case avoid calling getIdentityPublicKeys — instead populate the state with the
deterministic/mock identity keys (or a noop success) by calling
setManageIdentityFetched with the mock keys (e.g., for E2E_MOCK_IDENTITY_ID) or
skip to reset state; preserve the current error handling path for non-mock
networks so only real networks invoke getIdentityPublicKeys, and still call
updateState(setManageIdentityFetchError(...)) on real failures.
Contributor
Author
|
Follow-up on the mention triage: I addressed the remaining actionable CodeRabbit items in cc5d9fe. Changes included:
Validation:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.ts (1)
1374-1396:⚠️ Potential issue | 🟡 Minor
recheckDeposithas no mock-mode guard — real API calls would fire if the deposit-timed-out state is ever reached during E2E tests.The mock paths in
startTopUpandstartBridgenever callsetDepositTimedOut, sorecheckDepositis unreachable under normal mock flows. However, the deterministic guarantee relies entirely on that precondition holding — there is no explicit guard. If a test ever exercises this button,insightClient.waitForUtxoanddapiClient.waitForInstantSendLockwould fire against real infrastructure.🛡️ Suggested fix
async function recheckDeposit() { if (!state.depositAddress) { console.error('No deposit address available'); return; } + if (isE2EMockMode()) { + // Mock flows never time out, so this path should not be reached. + // Guard here to prevent accidental real API calls in deterministic tests. + return; + } + // Reset timeout state and start polling again updateState(setDepositTimedOut(state, false, 0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1374 - 1396, recheckDeposit currently calls real network methods (insightClient.waitForUtxo / dapiClient.waitForInstantSendLock) without checking mock mode, so E2E tests can trigger real API calls; add an explicit mock-mode guard at the start of recheckDeposit (e.g., check isMockMode or a similar flag from state/config) and early-return or use the existing mock clients when mock mode is active; also ensure any related handlers (startTopUp, startBridge) that toggle setDepositTimedOut still set or pass the same mock flag so recheckDeposit's guard is effective; reference the functions recheckDeposit, startTopUp, startBridge, setDepositTimedOut, insightClient.waitForUtxo and dapiClient.waitForInstantSendLock when making the change.
🧹 Nitpick comments (1)
src/main.ts (1)
626-633: Hardcoded mock key IDs are undocumented magic numbers coupled tocreateE2EMockIdentityKeys().
setDpnsKeyValidated(state, 1, ...)(Line 628) andsetManageKeyValidated(state, 0, 0, ...)(Line 875) hardcode the key IDs and security level that must match the mock key array fromcreateE2EMockIdentityKeys(). If the mock key fixture is ever changed (e.g., key IDs reordered or security levels adjusted), these validation blocks would silently produce wrong results without any compile-time signal.Consider exporting these IDs from
src/e2e-mock-constants.tsalongside the existing WIF constants:♻️ Suggested extraction to `src/e2e-mock-constants.ts`
export const E2E_MOCK_DPNS_WIF = 'cMockDpnsPrivateKeyWif'; export const E2E_MOCK_MANAGE_WIF = 'cMockManagePrivateKeyWif'; export const E2E_MOCK_IDENTITY_ID = '11111111111111111111111111111111111111111111'; +/** Key ID for DPNS validation — must match the CRITICAL/AUTH key in createE2EMockIdentityKeys() */ +export const E2E_MOCK_DPNS_KEY_ID = 1; +/** Key ID for manage validation — must match the MASTER key in createE2EMockIdentityKeys() */ +export const E2E_MOCK_MANAGE_KEY_ID = 0; +export const E2E_MOCK_MANAGE_SECURITY_LEVEL = 0; // MASTERThen in
src/main.ts:-import { E2E_MOCK_DPNS_WIF, E2E_MOCK_IDENTITY_ID, E2E_MOCK_MANAGE_WIF } from './e2e-mock-constants.js'; +import { E2E_MOCK_DPNS_WIF, E2E_MOCK_IDENTITY_ID, E2E_MOCK_MANAGE_WIF, E2E_MOCK_DPNS_KEY_ID, E2E_MOCK_MANAGE_KEY_ID, E2E_MOCK_MANAGE_SECURITY_LEVEL } from './e2e-mock-constants.js';- updateState(setDpnsKeyValidated(state, 1, privateKeyWif)); + updateState(setDpnsKeyValidated(state, E2E_MOCK_DPNS_KEY_ID, privateKeyWif));- updateState(setManageKeyValidated(state, 0, 0, privateKeyWif)); + updateState(setManageKeyValidated(state, E2E_MOCK_MANAGE_KEY_ID, E2E_MOCK_MANAGE_SECURITY_LEVEL, privateKeyWif));Also applies to: 873-880
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 626 - 633, The code hardcodes mock key IDs/security levels (e.g., setDpnsKeyValidated(state, 1, ...) and setManageKeyValidated(state, 0, 0, ...)) that must match the array returned by createE2EMockIdentityKeys(); export the corresponding key id and security-level constants (e.g., E2E_MOCK_DPNS_KEY_ID, E2E_MOCK_MANAGE_KEY_ID, E2E_MOCK_MANAGE_KEY_SECURITY_LEVEL) from src/e2e-mock-constants.ts (alongside E2E_MOCK_DPNS_WIF), then replace the numeric literals in main.ts with those exported constants so the validation calls (setDpnsKeyValidated and setManageKeyValidated) always reference the canonical fixture identifiers rather than magic numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main.ts`:
- Around line 1374-1396: recheckDeposit currently calls real network methods
(insightClient.waitForUtxo / dapiClient.waitForInstantSendLock) without checking
mock mode, so E2E tests can trigger real API calls; add an explicit mock-mode
guard at the start of recheckDeposit (e.g., check isMockMode or a similar flag
from state/config) and early-return or use the existing mock clients when mock
mode is active; also ensure any related handlers (startTopUp, startBridge) that
toggle setDepositTimedOut still set or pass the same mock flag so
recheckDeposit's guard is effective; reference the functions recheckDeposit,
startTopUp, startBridge, setDepositTimedOut, insightClient.waitForUtxo and
dapiClient.waitForInstantSendLock when making the change.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 626-633: The code hardcodes mock key IDs/security levels (e.g.,
setDpnsKeyValidated(state, 1, ...) and setManageKeyValidated(state, 0, 0, ...))
that must match the array returned by createE2EMockIdentityKeys(); export the
corresponding key id and security-level constants (e.g., E2E_MOCK_DPNS_KEY_ID,
E2E_MOCK_MANAGE_KEY_ID, E2E_MOCK_MANAGE_KEY_SECURITY_LEVEL) from
src/e2e-mock-constants.ts (alongside E2E_MOCK_DPNS_WIF), then replace the
numeric literals in main.ts with those exported constants so the validation
calls (setDpnsKeyValidated and setManageKeyValidated) always reference the
canonical fixture identifiers rather than magic numbers.
Add mock mode gated behind ?e2e=mock URL parameter that intercepts all platform API calls and returns fake data. This enables full UI flow testing without network access or real funds. Mock mode covers: create identity, top-up, manage keys, DPNS registration, and standalone DPNS flows. Mock helpers produce deterministic identity IDs, key data, and DPNS availability.
Add deterministic mock-mode tests (create, top-up, manage, DPNS) and optional live testnet tests (gated by env vars). Includes Playwright config, CI workflow with Chromium install, vitest exclude for e2e dir, and npm scripts for running headed/headless/live modes.
The npx playwright@1.52.0 approach only installs the CLI runner, not the @playwright/test package that playwright.config.ts imports. CI failed with ERR_MODULE_NOT_FOUND. Adding as devDependency and using local bin instead of npx.
… timing Top-up mock now pauses at detecting_deposit (same pattern as create flow) so test can assert deposit headline before advancing. Also added explicit visibility wait for contested checkbox before checking it.
The mock createE2EMockDpnsAvailability was missing the isContested field. Without it, shouldShowContestedWarning returns false, the checkbox never renders, and page.check times out. Now uses the real isContestedUsername logic to match production behavior.
page.check() waits for actionability checks that fail (likely element
obscured by label or CSS). Using click({force: true}) bypasses those
checks since we already assert visibility beforehand.
thepastaclaw
force-pushed
the
feat/playwright-e2e-testnet
branch
from
cc5d9fe to
250efc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/deterministic.spec.ts`:
- Line 66: The assertion fails because page.locator('.identity-id') matches two
elements; update the locator to target the exact element (e.g., use .first() or
a hasText filter) so Playwright's strict mode only matches one node. Replace the
failing call to page.locator('.identity-id') in the test and use
page.locator('.identity-id').first() (or page.locator('.identity-id', { hasText:
E2E_MOCK_IDENTITY_ID })) before calling toHaveText(E2E_MOCK_IDENTITY_ID) so the
assert on the completion screen is unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3934341f-e888-44bb-8da7-1312792ee713
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/ci.yml.gitignoreREADME.mde2e/deterministic.spec.tse2e/live.testnet.spec.tspackage.jsonplaywright.config.tssrc/e2e-mock-constants.tssrc/main.tssrc/types.tssrc/vite-env.d.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (5)
- src/vite-env.d.ts
- .gitignore
- vitest.config.ts
- src/e2e-mock-constants.ts
- playwright.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- .github/workflows/ci.yml
- src/types.ts
thepastaclaw
force-pushed
the
feat/playwright-e2e-testnet
branch
from
a63733c to
5a851e9
Compare
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
Adds Playwright E2E test infrastructure with two test modes:
Deterministic mock mode (CI-safe)
?e2e=mockURL parameterOptional live testnet mode
PW_E2E_LIVE=1environment variableChanges
src/main.ts: Mock mode implementation (~180 lines, gated behind URL param)e2e/deterministic.spec.ts: 4 deterministic UI flow testse2e/live.testnet.spec.ts: 4 optional live testnet testsplaywright.config.ts: Chromium config with Vite dev server integrationvitest.config.ts: Excludese2e/from unit test runs.github/workflows/ci.yml: Newe2ejob with Playwright install + runpackage.json: npm scripts for E2E (headless, headed, live, install)README.md: E2E usage documentation with env var referenceNote
Playwright is used via
npx playwright@1.52.0(not a dev dependency) to keep the install footprint minimal. The mock mode tests have not been verified locally (headless macOS machine without display) — CI will be the first real validation. Follow-up commits may be needed to fix test selector mismatches.Summary by CodeRabbit
New Features
Tests
Documentation
Chores
Validation
What was tested
build (20)build (22)e2eResults
build (20): passbuild (22): passe2e: passEnvironment