test(wallet-flows): harden launch signoff criteria and wallet preflight#3140
test(wallet-flows): harden launch signoff criteria and wallet preflight#3140
Conversation
✅ Deploy Preview for tangle-dapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for tangle-cloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for tangle-leaderboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Claude finished @drewstone's task —— View job PR Review CompleteReviewing PR #3140: test(wallet-flows): harden launch signoff criteria and wallet preflight Todo List
Code Review FeedbackOverall AssessmentThis is a well-executed PR that significantly hardens the wallet flow testing infrastructure. The changes demonstrate mature testing practices with robust error handling, comprehensive state validation, and thoughtful fallback mechanisms. Positive Aspects1. Robust Transaction Lifecycle VerificationThe new transaction verification system in
// Excellent implementation - handles edge cases well
const makeTxOutcomeOrBlockerCriterion = (flowId, blockerSelectors, blockerLabel) => ({
check: async (page) => {
if (await anyVisible(page, blockerSelectors)) {
return true; // Smart early exit for blocked states
}
return makeTxOutcomeCriterion(flowId).check(page);
},
});2. Mature Error Handling & FallbacksThe wallet preflight system shows sophisticated error recovery:
3. Comprehensive Documentation UpdatesThe documentation in Areas for Improvement1. Code Duplication in Verification Logic (scripts/agent-browser/cases/launch-ready-manual-signoff.cases.mjs:744-756)Several flows have very similar verification patterns that could be abstracted: // Found in FLOW-013, FLOW-016, FLOW-017 - consider extracting common pattern
const blockerSelectors = [
'text=/No data found/i',
'text=/No Registrations/i',
// ... more selectors
];
if (await anyVisible(page, blockerSelectors)) {
return true;
}Recommendation: Extract a 2. Magic Numbers Should Be Constants (scripts/agent-browser/run-wallet-flow-suite.mjs:30-31, 45-46)Several hardcoded timeouts and thresholds lack clear documentation: // Should be documented constants
const TX_OUTCOME_TIMEOUT_MS = 120_000;
const TX_OUTCOME_POLL_INTERVAL_MS = 1_500;
const MIN_LOCAL_BALANCE_WEI = 1_000_000_000_000_000_000n; // 1 ETH
const TARGET_LOCAL_BALANCE_WEI = 20_000_000_000_000_000_000n; // 20 ETHRecommendation: Group these into a configuration object or move to config file. 3. Complex Function Lengths (scripts/agent-browser/run-wallet-flow-suite.mjs:908-1131)The Recommendation: Break into smaller, focused functions:
4. Inconsistent Error Message FormatError handling varies between detailed structured messages and simple strings. Consider standardizing error objects with consistent fields (operation, reason, context). Security Considerations✅ Secure Practices Observed:
|
1 participant
Summary\n- harden wallet flow suite verification criteria for launch-ready signoff flows\n- add explicit blocker handling for migration dev/mock mode and service permission-gated states\n- improve tx lifecycle verification by opening/rechecking transaction drawer and canonical route fallback\n- pass through additional wallet preflight env flags in docker runner\n\n## Verification\n- yarn lint\n- wallet flow suite (full): 16/18 verified pass in latest complete run with only FLOW-007 + FLOW-016 failing pre-patch\n- targeted rerun after patch: FLOW-007/FLOW-013/FLOW-016 => 3/3 passed\n\n## Notes\n- agent success text can still report max-turn/timeouts while verification criteria pass; launch gate currently keys on verified criteria