01a034d6 - Keep the verification-call section visible after Completed and Failed - #1426
Conversation
|
EN: DE: DetailsHead Grok A: coverage of CI on this draft is skipped ( The PR stays draft until a reviewer grants the two deviations or the extra tests and baselines are added, and until CI has actually run. |
|
EN: DE: User-Entscheid: keep the declared CONTRIBUTING deviations for coverage of the whole settings screen and for Completed/Failed Playwright baselines. The mail-link dead end is fixed by the section change; overlay, bank and address paths are untouched. |
|
@marassteiner please review — head EN: DE: |
|
EN: Working on this now — job |
marassteiner
left a comment
There was a problem hiding this comment.
EN:
Changes requested: whole-file coverage of settings.screen.tsx and Completed/Failed handbook baselines are still missing, and the author's declared deviations are not a reviewer grant.
DE:
Änderungen nötig: die 100%-Coverage der ganzen Settings-Datei und die Handbook-Baselines für Completed/Failed fehlen weiter; die deklarierten Abweichungen des Autors sind keine Reviewer-Freigabe.
Details
Head 8c2d7db8b6d23fda985667388723c48ce95cb606 against develop. Logic of the verification-call section is sound. Conformity is not.
Blocking
-
src/screens/settings.screen.tsx(changed branch around lines 300–368). CONTRIBUTING requires 100% statement, branch, function and line coverage for every touched Jest-instrumented file. This file is incollectCoverageFrom. The new tests insrc/__tests__/verification-call-section.test.tsxcover only the verification-call states. Overlay, bank-account and address paths in the same file are untouched. A declared deviation is not a granted one; the author cannot grant their own deviation.Fix: bring the whole file to 100% on all four metrics and state the numbers in the pull-request description, or obtain a written grant from a reviewer on this pull request.
-
src/screens/settings.screen.tsx:309–329. The change introduces Completed-notice and Failed-notice+button visual variants. CONTRIBUTING handbook coverage must be complete: each visual variant needs a committed Playwright baseline and an entry inscripts/handbook/metadata.json.e2e/user-flows.spec.tsalready hassettings-main.png; that baseline does not cover Completed or Failed. This diff adds neither baselines nor metadata.Fix: add
toHaveScreenshotfor at least the open/consent, Completed and Failed variants, commit the baselines, updatescripts/handbook/metadata.json— or obtain a written grant from a reviewer.
Noted, not blocking
- CONTRIBUTING says a screen change should bring or update the matching full-stack test.
/settingsis already claimed bye2e-stack/specs/account.spec.ts, which does not assert the Completed/Failed notices. No new route. This is a should, not the coverage/handbook must.
The User-Entscheid comment on this pull request is from the author. It records product intent; it does not grant the deviations.
Do not merge until the blockers are fixed or a reviewer grants the declared deviations in writing.
|
@marassteiner both blocking points are implemented on head EN: DE: Details1. Coverage. One production change comes with it: Because a coverage number alone says nothing about what the tests assert, three mutation probes, each flipping one condition and leaving the code runnable:
2. Handbook. Three baselines committed under 3. Full-stack spec — not done. Gates, Node 20: Open, and not closable from this branch: the pull request is a draft, and |
…ailed The mail link to /settings?a=call found no target once phoneCallStatus was Completed or Failed. The heading now always renders, with a status notice and — for Failed — a button to request a new call.
CONTRIBUTING requires 100% statement, branch, function and line coverage for every touched file. The verification-call tests covered only their own states, so the bank-account, address, form-effect and overlay paths of the same file stayed uncovered. OverlayType and SettingsOverlay are exported because the NONE branch of the overlay switch cannot be reached through the screen, which only renders the overlay for a truthy type.
The spec merged with DFXswiss#1434 covers the three consent states. Completed and Failed are the two variants this pull request introduces, so they were still missing from the handbook. The reality declaration said a green run does not prove that Completed/Failed hide the section; after this change they no longer hide it, so the sentence is replaced by the limits the new images actually have.
3a54651 to
fe57920
Compare
Two mutations passed every test before this commit: removing ref={verificationCallRef} from the heading, and emptying any of the four effect dependency arrays. The first is half of the reported bug — the mail link /settings?a=call scrolls to that heading — and the second made the effects mount-only in the test.
|
@marassteiner update on head EN: DE: Correcting my previous comment. It described the pre-rebase head Details1. Coverage — Removing the fix itself fails 6 of 28 Jest tests and 2 of 5 Playwright cases. Seven mutation probes, each flipping one condition and leaving the code runnable:
The first two rows are worth naming: before commit 2. Handbook — two baselines added to your spec, A local handbook build reports One deviation, stated plainly. Point 3 stays open on purpose: A twin, reported not fixed: Gates on Open and not closable from this branch: no CI check has run on |
The heavy jobs run on pull_request: synchronize with draft == false. This branch was last pushed while the pull request was still a draft, so they were skipped; Ensure CI label cannot dispatch them because the head lives in a fork.
|
CI is green on EN: DE: Two things a green tick does not cover here: |
EN:
The verification-call section on Settings now stays on the page after Completed and Failed, so the mail link
/settings?a=callstill has a target. Completed shows a short notice; Failed shows a notice and a button that opens a new-call support request. Open statuses keep the existing consent form.DE:
Die Sektion Verifizierungsanruf bleibt nach Completed und Failed sichtbar, damit der Mail-Link
/settings?a=callein Ziel hat. Completed zeigt einen Hinweis; Failed einen Hinweis und einen Button für eine neue Anruf-Anfrage. Offene Status behalten das bisherige Formular.Open, and not closable from this branch:
marassteinerrequested changes on8c2d7db8; both blockers are implemented on1a043d1a, but only the reviewer can resolve the request.reviewDecisionis stillCHANGES_REQUESTED.Ensure CI labelrun on the superseded head053c277f— needs admin rights on this repository. It is red by design: the job refuses a fork head (GITHUB_TOKEN cannot workflow_dispatch a fork head) and asks for a human-appliedcilabel, which also needs admin (403here). Deleting the run and re-running it both return403for this account, and re-triggeringready_for_reviewwould only move the same failure onto the current head. The head itself is clean: 7 of 7 check runs on1a043d1aaresuccess, becausepr.ymltriggers onpull_request: synchronizewithdraft == falseand the push of1a043d1astarted them.user-flows.spec.ts-settings-main-chromium-darwin.png. It needs the full local stack (the api repository as a sibling checkout plus Docker), which this branch cannot provide. See the note below — it is stale ondevelop, not stale because of this pull request.Symptom (verbatim): für die Vereinbarung eines Telefonats hatten Sie mir einen link zugeschickt. Nach dem Öffnen sollte ich mögliche Zeiten auswählen können. Beim Anklicken des links komme ich auf keine Seite, auf der ich einen Termin angeben kann.
Scale: 1041 Completed accounts see nothing at the mail target; 51 Failed accounts are told by mail to act on that page while the section is hidden. The form itself still works: 17 accounts set a call time on 2026-08-24.
Smaller fix considered: Always render the section and replace the two dropdowns with a status notice at Completed and Failed — about 25 lines in one file, no API change. That is this PR.
Review blockers, implemented on
1a043d1a:src/screens/settings.screen.tsxis at 100 % statements, 100 % branches, 100 % functions, 100 % lines (was 40.9 / 46.15 / 26.31 / 41.34). 28 tests in 2 suites, both green.e2e/settings-verification-call.spec.tsgained the Completed and Failed variants with two committed baselines; the three consent variants and themetadata.jsonkey came from 01a04085 - Handbook coverage for the Settings verification-call choice #1434, which merged intodevelopon 2026-08-27 at 09:33 while this branch was open. This pull request therefore extends that spec instead of adding a second one.Review point 3 (a should) stays open on purpose:
e2e-stack/specs/account.spec.tsalready claims/settingsand is not extended to assert the notices. No new route is added.Round check (1a043d1):
Twins:
rg 'useAnchor\('oversrc/returns exactly two call sites: this one andaccount.screen.tsx:115(useAnchor('recommendation', recommendationsRef, …)). The second one has the same shape — its anchor target ataccount.screen.tsx:530renders only inside{isKycLevel50 && …}, so a deep link to/account?a=recommendationfrom an account below level 50 lands nowhere. Whether any mail sends that link is decided in the api repository, not here, so it is reported and not fixed in this pull request. No other anchor exists;rg '\?a='oversrc/finds no in-app link, only the mail target.New surface: the rebase onto
76a4fc79, the two Playwright cases in the spec merged with #1434, the reality-declaration paragraph indocs/test-architecture.md,src/__tests__/settings-screen.test.tsx(767 lines) and 37 added lines insrc/__tests__/verification-call-section.test.tsx. All of it was read back; the two exports insettings.screen.tsxare the only production change beyond the section itself.Previous findings: the two blockers of the 2026-08-26 review hold as implemented, evidenced by the numbers below. Two findings of my own review lanes were measured and fixed in
053c277f— removingref={verificationCallRef}and emptying an effect dependency array both passed all 25 tests before; both fail now. Three lane findings about the pull-request description (stale baseline names, a misquote of CONTRIBUTING, numbers from the pre-rebase head) are fixed by this rewritten description. Four suspicions raised by the lanes were withdrawn after being checked against the code: button colour, a Prettier gate for TypeScript, a lint rule against nested ternaries, and Markdown re-wrapping — none of those gates exist in this repository.Final pass (1a043d1):
Coherent: every file serves the one title — the section itself, three translations, the tests that pin it, and the two handbook variants that show it. Two empty commits,
153265e5and1a043d1a, carry no diff at all; they exist because the heavy jobs only run on a push made while the pull request is not a draft. They are kept rather than rebased away, because another rebase is another chance to lose a fix silently.Nothing extra: no API change, no new route, no new spec file, no second
metadata.jsonkey, and the open/consent baseline was not rebuilt because #1434 already committed it. The one production change beyond the section isexportonOverlayTypeandSettingsOverlay; the reason is spelled out below, and it is a deviation from what CONTRIBUTING literally asks for.Sources closed: the customer mail (symptom quoted above); the hide condition in
settings.screen.tsx; the review of 2026-08-26 — blocker 1 and 2 implemented, point 3 declined with a reason; PR #1434 as the source of the spec this change extends; and three independent review lanes whose findings are listed under Previous findings.Skipped check (K1.13): Three workflows have no check on this head, each for a reason outside the diff:
Ensure CI labelonly fires onready_for_reviewand the head was pushed after that,codeql.ymldeclares its job asAnalyze (${{ matrix.language }})and actually reported asAnalyze (actions)andAnalyze (javascript-typescript), both green, andMain only from developapplies to pull requests targetingmain, while this one targetsdevelop.CI on
1a043d1a: 7 of 7 green — Build and test (5m57s), Analyze (actions), Analyze (javascript-typescript), CodeQL, review, Build handbook image + container smoke, Full-stack E2E. One caveat on the last one: it finished in 5 s, which is the known no-op path — without the api checkout credential the job reports success without starting the stack. Its green tick is therefore not evidence.Details
The mail to
/settings?a=callnever expires. After Compliance setsphoneCallStatusto Completed or Failed, the settings screen used to drop the whole section, including the heading the anchor scrolls to. The page loaded, the customer saw nothing at that spot.This change renders the section whenever
isUserLoadingis false. Completed shows only the notice. Failed shows the notice and a button to/support/issue?issue-type=VerificationCall&reason=RepeatCall. Every other status, including unset, keeps the existing consent dropdowns. Effects andupdateCallSettingsare unchanged. No API change.Coverage of
src/screens/settings.screen.tsxMeasured with
--collectCoverageFrom='src/screens/settings.screen.tsx'over both suites:Test Suites: 2 passed, 2 total/Tests: 28 passed, 28 total.src/__tests__/settings-screen.test.tsx(new, 767 lines) covers what the verification-call suite does not: the four form effects in both directions, the bank-account list including loading, absent accounts, the label and DEFAULT fallbacks and every menu action, the address list including the custody filter, the ACTIVE tag and all four menu actions, the layout title and itsonBack, the Danger Zone, and all sevenSettingsOverlaycases with theironConfirm,onCancel,onEditandonSubmitcallbacks.The one production change beyond the section, and why it deviates from CONTRIBUTING:
OverlayTypeandSettingsOverlayare now exported. Thedefaultbranch of the overlay switch cannot be reached throughSettingsScreen, which renders the overlay only for a truthyoverlayType, soOverlayType.NONEnever arrives there. CONTRIBUTING says to delete a line that genuinely cannot be exercised rather than exclude it from the measurement. Deleting this one is possible only by narrowing the prop type toExclude<OverlayType, OverlayType.NONE>and casting at the call site, because the switch has to stay exhaustive for the compiler — that is more production code touched than twoexportkeywords. The export is the smaller change, and it is a conscious deviation, not an oversight. Say the word and it becomes the narrowed prop type instead.Counter-probes. Removing the fix itself (
git apply -Rof the production hunk ofd3ef0612) fails 6 of 28 Jest tests and 2 of 5 Playwright cases. Beyond that, each probe below flips one condition and leaves the code runnable, because a deletion probe only proves a fragment is present:ref={verificationCallRef}removed from the heading}, [selectedLanguage]);→}, []);}, [selectedPreferredPhoneTimes]);→}, []);}, [acceptCall]);→}, []);filter((a) => !a.isCustody)→filter((a) => a.isCustody)account.default ? 'Default' : undefined→!account.default ? …userAddress.address === user?.activeAddress?.address && setWallet()→!==The first two rows are the reason for commit
053c277f: before it, both of those mutations passed all 25 tests. Removing therefis half of the reported bug — the mail link scrolls to that heading — and an emptied dependency array made every effect mount-only in the test, because the suite mocksreact-hook-formwith a staticuseWatch. My own review lanes found both; I measured them before accepting them, then closed them with two tests and re-measured.The probes from the earlier round still hold for the verification-call suite: restoring the old Completed/Failed hide condition fails 2 of its original 4 tests, swapping the Completed and Failed branches fails 2 of 4.
Handbook
e2e/settings-verification-call.spec.tswas merged intodevelopwith #1434 on 2026-08-27 (e73764f6), covering the three consent states. This pull request adds the two variants it is actually about:settings-verification-call.spec.ts-settings-verification-call-04-completed-chromium-darwin.pngsettings-verification-call.spec.ts-settings-verification-call-05-failed-chromium-darwin.pngWritten once with
--update-snapshots(2 written), then the whole spec re-run without it:5 passed, so the three baselines from #1434 reproduce here too.scripts/handbook/metadata.jsonkeeps its existing key and title; only the description grows to name all five variants.docs/test-architecture.mdis corrected in the same commit: its reality declaration said a green run does not prove that "Completed/Failed hide the section" — after this change they no longer hide it.A local handbook build (
node scripts/handbook/build.js docs/handbook/build) reportswrote 228 screenshots, 9 docs, 4 assetsand carries both new files inmanifest.json. The one stderr warning it prints, an orphanedcompliance-bank-tx-returnentry, predates this branch.The spec answers every request itself and passes nothing through to a real backend;
REACT_APP_API_URLpointed at a dead port during the run. Unmatched/v1/**and/v2/**calls are fulfilled with 501. Note that the spec does not assert the set of unexpected requests — it has no such collector, and this change does not add one.settings-main.pngis stale, and this pull request does not fix it.e2e/screenshots/baseline/user-flows.spec.ts-settings-main-chromium-darwin.pngwas last written on 2026-01-06 (#844). It shows neither the verification-call section nor the "Danger Zone" heading, both of which exist ondeveloptoday — so it was already stale before this branch, and regenerating it would pull two unrelated drifts into this diff. It also needs the full local stack. The five mocked variants of the same screen cover the change itself.Gates run locally (Node 20, the version
pr.ymlpins), all on053c277f:npm run lintnpm run format:md:checknpm test(full)Test Suites: 1 failed, 113 passed, 114 total/Tests: 1 failed, 1770 passed, 1771 totalnpm run build:devnpm run widget:devnpx playwright test e2e/settings-verification-call.spec.tsThe failing suite is
realunit.screen.test.tsx, which expects1,000where the machine renders1.000. Control run of that same file on the unmodified commit44ba2432:Test Suites: 1 failed, 1 total/Tests: 1 failed, 21 passed, 22 total;git log 44ba2432..origin/develop -- src/__tests__/realunit.screen.test.tsx src/screens/realunit.screen.tsxis empty, so the control still applies to this base. It is a locale difference, not a regression from this branch.What is not verified here
Full-stack E2Eis green in 5 s, which is the no-op path: without the api checkout credential the job reports success without starting the stack. It proves nothing about this change.153265e5and1a043d1a. Both exist only to start CI after leaving draft; removing them means another rebase, and a rebase is another chance to lose a fix silently./account?a=recommendationis decided in the api repository; the twin anchor described above is reported, not fixed.