fix(testing): deterministic canary session-proposal timing#5710
Open
tomiir wants to merge 1 commit into
Open
Conversation
The "Receive Session Proposal Timing" Grafana canary alert fired as a false positive because its metric (pairingReceiveSessionProposal) was captured by matching WalletConnect SDK console.log strings on the external wallet page. When those logs changed the metric silently stopped emitting, the panel went blank, and the gap was evaluated as a >7000ms breach. Replace the brittle console-log parsing with a deterministic signal: - Add WalletPage.waitForSessionProposal() — waits on the owned session-approve-button testid (same selector as the approve action, so a selector change fails the run and pages instead of silently dropping the metric). - Fixture now emits two deterministic timings: sessionProposalReceived (connect -> proposal rendered) and sessionProposalApproved (approve round-trip), replacing receiveSessionProposal / pairingReceiveSessionProposal / sessionProposalVerification. - Capture interval timestamps immediately on await resolution to avoid skew. Monitoring must repoint its query to timing.sessionProposalReceived. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
9 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 79d1416 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Visual Regression Test Results ✅ PassedChromatic Build: https://www.chromatic.com/build?appId=6493191bf4b10fed8ca7353f&number=961 👉 Please review the visual changes in Chromatic and accept or reject them. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the laboratory canary session-proposal timing metrics deterministic by removing brittle WalletConnect console-log parsing and replacing it with a wallet-UI-driven signal exposed via @reown/appkit-testing.
Changes:
- Added
WalletPage.waitForSessionProposal()(approve-button visibility) as a deterministic “proposal received” signal. - Updated the laboratory wallet fixture to emit new timing metrics (
sessionProposalReceived,sessionProposalApproved) and removed console-log-based timings. - Added a changeset bumping
@reown/appkit-testingwith the new helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/testing/src/pages/WalletPage.ts | Adds a deterministic session-proposal wait helper and refactors request-button selection into a shared helper. |
| apps/laboratory/tests/shared/fixtures/w3m-wallet-fixture.ts | Removes console-log timing capture and records deterministic session proposal received/approved timings. |
| .changeset/canary-session-proposal-timing.md | Publishes the new testing helper via a patch changeset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+88
| sessionRequestButton(variant: string) { | ||
| return this.page.getByTestId(`session-${variant}-button`) | ||
| } |
Comment on lines
29
to
33
| timeStart('walletPage.connectWithUri') | ||
| await walletPage.connectWithUri(uri) | ||
| // Capture immediately on resolution — nothing awaited in between — to avoid skewing the interval. | ||
| const connectionInitiated = new Date() | ||
| timeEnd('walletPage.connectWithUri') |
Contributor
📦 Bundle Size Check✅ All bundles are within size limits 📊 View detailed bundle sizes> @reown/appkit-monorepo@1.7.1 size /home/runner/work/appkit/appkit > size-limit |
Contributor
Coverage Report
File CoverageNo changed files found. |
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
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.
Why
The
AppKit Receive Session Proposal Timing > 7000msGrafana canary alert paged as a false positive (see the burst of P3 escalations on 2026-07-14). Root cause: the queried metric,HappyPath.sign.timing.pairingReceiveSessionProposal, was captured by matching WalletConnect SDKconsole.logstrings on the external wallet page (core/pairing/pairing,resolving attestation,verifyContext). When those internal logs changed format/level the metric silently stopped emitting → the panel went blank → the gap was evaluated as a>7000msbreach.The fix removes the brittle path and replaces it with a deterministic signal.
What changed
packages/testing(@reown/appkit-testing)WalletPage.waitForSessionProposal()— resolves when the wallet renders the request UI (approve button visible). It reuses the same ownedsession-approve-buttontestid asperformRequestAction(extracted intosessionRequestButton()), so a selector change breaks the approve flow and this wait together → the run fails and pages via the success/failure canary, rather than silently dropping just the timing metric.apps/laboratorycanary fixture (w3m-wallet-fixture.ts)page.on('console')block (the only console-log-gated timing capture in the suite).receiveSessionProposal/pairingReceiveSessionProposal/sessionProposalVerificationwith two deterministic timings:sessionProposalReceived—connectWithUriresolved → proposal rendered in wallet (the metric the alert should track)sessionProposalApproved— proposal rendered → session approved (approve round-trip; the boundary split)awaitresolution (no work in between) to avoid skew.Required monitoring-repo follow-up
Repoint the Grafana query (
cloudwatch.libsonnet) fromtiming.pairingReceiveSessionProposal→timing.sessionProposalReceived. Until then the panel stays blank (timing alerts already resolve gaps toOK, so no false page in the interim). New baseline reads lower — it excludes the approve round-trip — so retune thresholds against real data once it flows.Notes / known limitations
sessionProposalReceivedincludes the test wallet's render time; the pure relay/SDK delivery leg needs a wallet-side signal (owned markers inreact-wallet-v2) — tracked as a separate follow-up.sessionProposalVerification); Verify correctness is still covered by the pass/fail verify canaries.Verification
pnpm build && (cd apps/laboratory && pnpm typecheck)— not run in the authoring workspace (deps not installed); relying on CI.sessionProposalReceived+sessionProposalApprovedpresent intimingRecords; old items absent.🤖 Generated with Claude Code