Release: develop -> main - #1201
Merged
Merged
Conversation
…rule in CI (#1197) * fix(safe): label a foreign write mandate as view only, and guard the rule in CI The picker labelled by access level alone. A write grant on someone else's account therefore appeared exactly like one's own — fully usable — and then dropped the transaction area without a word once selected. Label and behaviour now share one predicate, so the list cannot promise what the screen refuses. That predicate is also the only thing standing between a mandate holder and an order booked against the wrong Safe, and it went wrong twice during review. Its only coverage was a Playwright run, which is explicitly not a CI gate. It has a unit test now: own plus write acts, own narrowed to read does not, foreign does not at either level, legacy acts. Moving it out of the hook is what makes that testable — it is plain logic with no React in it, and importing the hook pulls in ESM that Jest will not parse. * style(safe): remove a stray comment opener left by the extraction Pulling canTransact apart left an empty /** above the replacement comment. Still one valid block, so nothing complained — but every tooltip over canTransact showed the leftover opener as part of the text. * test(safe): show what a write mandate over a foreign Safe looks like The picker fix is about exactly one state — a write grant on someone else's account — and no baseline showed it. The only evidence was a sentence in the pull request saying it had been checked by hand. A third account carries that mandate now, and the fourth shot is that state: labelled view only, holdings visible, no transaction area, despite the grant saying write. The test insists on both before taking the picture.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 28, 2026 11:24
* Add Bank Frick personal IBAN buy parameter * Map Bank Frick URL selector to API enum * Fix personal IBAN public parameter contract * Clarify personal IBAN integration names * Preserve empty personal IBAN web attribute * Rerender personal IBAN web attribute changes * Scope web attributes to initialization * Harden personal IBAN selector handling Review of the initial implementation surfaced several ways a customer could request a personal IBAN and silently end up with the default bank instead. The selector is no longer copied into component state. It is derived on every read from its actual source - the URL search in the browser, the live widget property when embedded - so browser back/forward, redirects, re-authentication and widget attribute changes all take effect without a lifecycle to keep in sync. This removes the session storage entry, the logout heuristic and the explicit clearing, all of which could drift from the source. The web component now applies attribute and property changes after the initial connection again, including removal, and the app router is created once per embedding so an attribute change no longer sends the customer back to the start route. Frick errors are mapped from the API tokens to translated, cause-specific messages, kept separate for the buy flow and for stored payment detail reconstruction. The feature specific mapping only applies when the customer actually set the selector - it previously also caught ordinary EUR bank transfers and replaced the KYC dialog with an unusable retry hint. The extra bank row is limited to personal IBANs, relative URLs merge an existing query instead of appending a second one, overlapping quote fetches cannot show stale data, and failed invoice or receipt downloads surface a visible per row error instead of being swallowed. * Align personal IBAN error tokens with the API Two stored-detail tokens were renamed on the API side and never followed here, so the checks never matched and the customer saw the raw token instead of a translated message. Both paths are reachable in normal operation: a personal IBAN deactivated by an operator, and one reassigned by an account merge. Two further mismatches came out of a full comparison against the API enum: a check for a value the API never sends, and a value placed in the wrong mapper - it can only occur while requesting a quote, never while reconstructing stored payment details. One value the API does send was not covered at all. Tests now pin every token explicitly, including negative cases for the retired spellings, so a rename on either side fails here instead of silently degrading to an untranslated message. * Only promise a personal IBAN in the customer's name where that is true CHF accounts at Yapeal are genuinely held by the customer. EUR accounts at Bank Frick are not: the account stays with DFX and the customer gets an IBAN assigned to them alone. The self-service screen promised the customer's own name regardless, and it defaulted to EUR when no currency was given — reachable today through the sitemap link, which passes none. Until this feature there was no EUR provider, so the default was harmless. A missing currency is now a visible error instead of a silent default, and the confirmation text states what is actually true for the currency at hand. The promotional block on the buy screen already excluded EUR and is unchanged. * Consume the published SDK types for the personal IBAN selector The SDK release that carries these types is out, so the local bridge types can go. personalIbanProvider is now the SDK enum instead of a string, and the buy response type no longer needs a local extension for the bank field - the compiler enforces the contract with the API rather than a hand-kept copy. This also moves the app from react 1.5.0 to 1.6.0, which carries two unrelated SDK changes (#189, #193) that come along with the version. * Verify the personal IBAN response and remove the dead ends A review of the complete diff found that the app committed whatever the quote returned. Against an older API the selector is stripped from the request, so a customer who asked for a personal IBAN silently received ordinary bank details — and the legacy path names the customer as the account holder, although Bank Frick keeps the account with DFX AG. That is the mismatch a sending bank flags during payee verification. An explicit Frick request is now checked against the response: it must be marked as a personal IBAN, come from Bank Frick and name DFX AG. If it does not match, or if the offer cannot carry a personal IBAN at all, the payment details stay hidden behind an explicit acknowledgement instead of being substituted without a word. The acknowledgement resets whenever the selector or the quote inputs change. Two states dead-ended: a missing KYC level and an unrecognised selector both offered only a Retry button that reproduced the same failure. The KYC case now routes through the normal hint with its working action and keeps the feature-specific wording; the unrecognised selector offers continuing without it, or leaving. Further, from the same review: - the selector no longer survives a logout, in the URL or as a widget property, so it cannot leak into the next customer's purchase - external login callbacks forward only the selector instead of copying the entire live query string - changing a quote input invalidates the pending request immediately rather than after the debounce, so an obsolete response cannot win - the bank row appears only for a verified personal IBAN, restoring the previous screen for customers who never use the selector - a failed document download shows the real error instead of a substituted placeholder * Close the gaps the previous round left behind A fresh review of the reworked state found six defects, four of them introduced by the previous round's own fixes. Removing the dead ends created a new one: "Continue without personal IBAN" sent the plain quote but left the error on screen, so the customer stayed where they were. Invalidation now follows the effective selector, which clears the error before the standard quote arrives — and the test no longer stops at the outgoing request, it asserts that the payment details are actually reached. The logout suppression for embedded widgets could not be lifted. It compared the selector value, and a host reasserting the same value for the next customer changes nothing about that value, so the selector stayed silently disabled. Every attribute and property write now bumps a revision counter, and the suppression follows the counter rather than the string. The logout cleanup required an observed transition from logged in to logged out, which never happens when the persisted token is already expired at mount. An expired session is now distinguished from a first visit; only the latter keeps the selector through the login redirect. Also: clearing the amount discards the pending quote instead of letting its response arrive for an empty form, and sitemap links carry the selector along instead of dropping it. * Tie the selector to an observed customer boundary The last two rounds tried to recognise "a different person is using this app now" from the session state, and got it wrong three times. The rule is explicit in the code now, and the tests state it from the customer's side. The selector stops applying only when this app instance actually observed a customer boundary: a logout, or a change of the authenticated identity. An expired token found at mount is not a boundary — that is the same person coming back and being asked to log in again, and the previous round silently dropped their request. Neither is a first visit without a token, nor credentials that a host supplies together with the selector. After a genuine boundary the suppression now lasts for the lifetime of the mounted app, standalone as well as embedded. Otherwise the browser Back button reaches an older, selector-bearing entry, and because the login flow deliberately carries the selector, the next customer inherits it. It is lifted only by a new explicit intent: a navigation carrying the parameter, or a host write, which the revision counter already models. Two more from the same review: - Retry on a transient issuance failure sent no second request at all — the debounce discarded the structurally identical value. It now carries a retry counter, like the info screen already did. The test clicks the button instead of only asserting it exists. - Quote invalidation compares a canonical signature rather than raw form values, so retyping 300 as 300.0 no longer discards a valid response. * Bind an incoming selector to the credentials it arrived with A widget host that opens the app for a new customer supplies that customer's session together with the selector. The app recorded both as already seen before the wallet had installed the session, so the customer still present in local storage was observed first and the incoming selector was discarded as belonging to someone else — silently, with ordinary bank details as the result. Credentials and selector that arrive together now stay pending until those credentials have established their identity, and are then taken into account as one. Nothing is inferred from whichever identity happens to initialise first. The same reasoning fixes the React integration. Reasserting the selector after a customer boundary worked only for the Web Component, because only it produced the internal write counter. A React host expresses the same intent by supplying the selector together with changed credentials, and that now counts as fresh intent. The counter stays internal to the Web Component bridge — it was exposed in the widget parameters and is removed there, since a revision an integrator has to maintain by hand is a contract nobody asked for. Quote invalidation now signs the provider that the request actually carries rather than the live selector, so toggling it on an offer that cannot carry it no longer throws away a valid pending response. * Make the fallback fetch what it promises "Continue without personal IBAN" only revealed the response that had just been rejected. Against an older API that response is the personal IBAN with the customer named as recipient — the very payee mismatch this PR exists to prevent, shown to a customer who explicitly chose the plain alternative. Both purchase flows now discard the rejected response, suppress the selector and wait for a new selector-free one; the tests assert the standard IBAN and DFX AG as recipient, and assert the personal IBAN is gone. The previous round's pending state could never resolve for credentials supplied after mount, because this app applies supplied session and address props during initialisation only. The selector stayed hidden forever. Pending now applies only where the credentials are actually consumed, which is where it fixed a real problem; elsewhere a selector supplied with new credentials counts immediately. The initialisation-only contract is written down in the README instead of being implied. The exact-price synchronisation compared against the raw input, so a customer typing 300.0 invalidated the response the app had just received and waited for another. It now uses the same canonical signature as the request. The Bank Frick branch on the self-service screen was unreachable: that endpoint only knows Yapeal, and Yapeal only does CHF. It is removed along with its translations and a test that asserted a state production cannot enter. The README no longer claims the API rejects unknown providers — they never reach it. * Ask instead of guessing who the selector belongs to Five rounds tried to recognise from events that a different person had taken over the app: pending credential intent, observed transitions, suppression scoped to the mount, restoration on navigation, a revision counter in the Web Component bridge. Each round closed one edge case and opened the next — most recently an invalid session that left the selector pending forever, and a suppression that a reload simply forgot. All five mechanisms are gone. The selector stays what it was, a value read from the URL or the widget property. When it is applied to a quote, the authenticated account it applied for is recorded, in storage that survives a reload. Before applying it again the app compares: the same account follows the stored decision, a first use binds it, and a different account is asked to confirm or decline. The case is genuinely undecidable from the address alone — the same URL can mean a leftover from the previous customer or a deliberate request by the current one. So it is now a question to the customer instead of a guess, which is what the rest of this flow already does. The four guarantees the earlier rounds established each have a test: an embedded customer arriving over a still-valid previous session, an expired token surviving the login redirect, logout followed by Back and a reload, and a malformed session that can still be left. Also: the failure state of the selector-free fallback offers Close beside Retry, the error mapping compares SDK members instead of hand-written strings, and the test that asserted README prose is removed — this repository has no precedent for it, and the companion backend PR dropped the same construct. * Confirm the personal IBAN instead of storing who it belonged to The identity binding from the previous round carried its own problems: a single decline locked the customer out for good, one stored decision per provider overwrote the previous customer's without a trace, and an unreadable or unexpected stored value was treated as a valid empty answer. All three follow from keeping a durable record of who a selector belonged to. That record is gone. A personal IBAN means a real account is opened at a bank and cannot be revoked, so the app asks the customer to confirm it — once per tab and authenticated customer, remembered in session storage so a reload does not ask twice. A different customer in the same tab is asked separately, a new tab asks again, and nothing outlives the tab. Declining applies to this occurrence only; a selector arriving anew asks again. If session storage is unavailable or holds something unexpected, the app asks. An answer it cannot read is not an answer. Customers who never set the selector see no dialog, no storage access and no change in quote timing — the previous round's authentication gate had started delaying their quote too. The tests now drive the scenarios instead of asserting a mocked end state: the dialog is clicked and the outgoing request inspected, the reload and the Back navigation are actually performed, and session storage is made to throw. Where the test setup cannot drive a scenario — the SDK's authentication is globally mocked in this repository — the report says so rather than asserting a proxy. The error-token test now reads the installed SDK enum instead of inventing values and asserting the real tokens do not match them. * Say who the account belongs to, and stop storing the answer The confirmation told the customer an account is opened for them. For Bank Frick that is untrue: the account is held by DFX AG and only the IBAN is theirs. In a change that exists because of a payee name mismatch, that sentence was the wrong one to get wrong. It now says what happens — a transfer IBAN of their own, on an account belonging to DFX — in German, French and Italian. The Yapeal wording stays as it is, because there the account really is the customer's. The answer is no longer written to session storage. That storage did not deliver the lifetime the design assumed — a tab opened from another same-origin tab inherits a copy of it — and a read that threw while writes succeeded could leave a customer unable to get out of the confirmation. It is now held in memory for the running instance: a reload asks again, which is proportionate for an account that is created for real and cannot be revoked. Nothing is serialised, so there is no shape to validate and no failure to fall back from. A decline applies to the occurrence in front of the customer. Any fresh arrival of the selector asks again, including a changed React prop — the React integration previously collapsed all its occurrences into one and would have gone quietly to ordinary details after an earlier decline. Two behaviours cannot be tested here: the selector surviving a login redirect after an expired token, and a logged-out customer's selector not reaching the next one after Back and a reload. This repository mocks the SDK's authentication, so no honest automated test reaches that boundary; both are written down as manual checks instead of being approximated by a test that only looks like proof. The Playwright visual now confirms the prompt before waiting for the bank details it could no longer have reached. * Check the selector before warning about a bank account The confirmation about a real, non-revocable account appeared for any non-empty selector, before the provider was recognised and before applicability was checked. A customer who mistyped the parameter was warned about a Bank Frick account that would never have been created, and a valid request on an offer that cannot carry a personal IBAN got the same prompt. Recognition and applicability now come first; only a valid, applicable Bank Frick request reaches the confirmation, and an unknown provider keeps the local rejection with its acknowledged fallback. A decline outlived the purchase it was given for in a mounted widget, so every later purchase quietly received ordinary details. The occurrence now includes the purchase, and a new one asks again. The selector staying in the address bar is deliberate and unchanged: an issued personal IBAN is a durable property of the customer, so reusing it for a later eligible EUR purchase is correct. The currency message claimed personal IBANs are EUR-only. Yapeal issues them in CHF; only the Bank Frick path is EUR. The copy guard has the right shape now. It checked complete sentences in three translation files, so better wording would have failed it while wrong wording in the component would have passed. It now renders the prompt per language and asserts the fact: the account is attributed to DFX, and no text claims it belongs to the customer. The README prose test is removed again — it was reintroduced under a new name after being rejected, and this repository has no precedent for asserting documentation in Jest. * Stop making customers without the selector wait Both quote screens waited for wallet initialisation before requesting a quote at all, not only when a personal IBAN was involved. A customer who never sets the selector had no reason to wait, and the info screen could stay blank if initialisation stalled. Readiness is now checked only inside the Bank Frick branch, and its settling no longer triggers a second request. The test meant to cover this had set initialisation to finished, so it never exercised its own claim; both screens now test it with initialisation pending. Declining the confirmation on the main buy screen asked for the same decision a second time and explained it with a message about personal IBANs being EUR-only — which is both wrong and unrelated. The two buy flows had drifted apart; the main screen now uses the same condition as the info screen, which was already correct. The occurrence identity no longer falls back to an inferred counter when a caller omits it. That fallback turned a caller's mistake into a silent customer outcome: the inferred value does not change on a re-render, so a previous decline stayed in force and ordinary details went out unasked. It now fails loudly, and identity is owned by the two bridges that can produce it correctly. * Let the two integrations agree on what a new request is The Web Component treated a property or attribute write with an unchanged value as a fresh selector request, while an unchanged React prop did not. So the same customer action meant different things depending on how the app was embedded, and in the Web Component a declined confirmation could reappear at once for the same purchase. Both now take a new request from a changed value — including one appearing or disappearing — or from a new navigation, and not from a write that changes nothing. The same-value trigger existed to let a host reassert the selector for a new customer back when suppression was lifetime-scoped; the confirmation is bound to the authenticated customer now, so a customer change asks again on its own. The mechanism outlived its reason. The exported `usePersonalIban` hook had no consumer and carried comments describing an integration that does not exist. Both are gone; deriving the selector stays private to the confirmation hook. The payment-details screenshot cropped to amount, IBAN, BIC and bank, so it did not cover the recipient — the field this change is about. The crop now includes it and the test asserts DFX AG as text, which holds even before the baselines are regenerated. Both baselines need regenerating on a machine that can run the stack; the confirmation one does not exist yet. * Restore the standalone entry point The widget build swaps src/index.tsx for the web component entry and restores it afterwards, keeping the original in src/index.bak.tsx while it runs. The previous commit captured that swapped state: the standalone entry contained the web component bootstrap, and the real one was committed as the backup file. A normal build would have compiled without an error and served an empty page — the entry registered the custom element and never mounted the app into #root. Nothing in lint, the unit tests or either build can catch that, which is why it went out. src/index.tsx is identical to develop again, the web component bootstrap lives only in src/index-widget.tsx, and the backup file is gone. * Make the quote tests wait for the condition, not the clock One personal-IBAN test failed in a full run that took 244 seconds because the machine was busy, and passed in 15 seconds when it was not. Flakiness is a defect here, not an environmental quirk: what fails under load can fail in CI. The race-protection tests waited out a fixed 500 ms for a slow in-flight request to settle and used arbitrary three-second ceilings. They now resolve that request explicitly and wait for the condition itself, so the outcome no longer depends on how fast the machine happens to be. Raising the timeouts would have hidden the dependency instead of removing it. The export-surface test is gone. It asserted that a module does not export a symbol — implementation surface rather than behaviour, with no precedent in this repository, and the third construct of its kind removed from this change. What matters is covered by the confirmation and integration tests. * Keep the feature wording out of a declined quote After the customer declines the confirmation on the info screen, the request correctly goes out without the provider, but the error mapping was still keyed on the raw selector. A declined customer who then hit KycRequired read "Personal IBANs require KYC level 50" instead of the generic KYC dialog with its working action. It is now keyed on the provider the request actually carries, as the interactive buy screen already did. That is the third time the two flows drifted apart and the second time this particular scope was wrong — the first time it took the KYC dialog away from customers who never used the feature at all. Both flows were compared end to end afterwards: request construction, error scope, confirmation, decline, fallback and every resulting state. The differences that remain follow from what the two routes are — an editable form with a debounce and an exact-price cycle versus one fixed request from route parameters, a back navigation versus a Close action — and each is recorded with its reason. * Show why the invoice could not be created The PDF invoice button caught every failure and wrote it to the console. The API now returns specific reasons for exactly this path — a personal IBAN that was deactivated, reassigned, mismatched, or whose bank stopped accepting payments between the quote and the click — and this app already had translated messages for them. The customer saw the button stop loading and nothing else. The error is now rendered: known tokens through their translated message, anything else through the API's own text, with no placeholder substituted for a missing one. The other consumers of these endpoints were checked; the transaction list already handled them correctly. * Drop the guards that cannot fail and mark the false positive The analysis bot flagged three conditions on isRunning that always hold. They are synchronous statements inside the effect body, where the flag has just been set and nothing can have reset it — leftovers from the race work. The asynchronous guards in then, catch and finally stay: those can observe a stale run and are the actual protection. CodeQL reports clear-text storage of sensitive information at the selector read. It is a false positive: the parameter carries a provider identifier — frick — never an IBAN, the value is normalised to the SDK enum, and neither the hook nor its context persists anything. The confirmation decisions live in an in-memory map keyed by customer id and are gone on remount. The read carries a scoped suppression with that reasoning; the alert itself is left for a maintainer to judge.
TaprootFreak
previously approved these changes
Jul 28, 2026
* fix(pages): restore the cache semantics four assets lost public/_headers promises to replicate what the previous CDN served, but four assets fall through it. robots.txt, favicon.ico and logo.png have no rule at all and take Cloudflare's default instead of the values the upload step used to set. The WebAssembly chunk of the main build lands in the build root, because config-overrides.js set webassemblyModuleFilename only in the branch guarded by CUSTOM_CHUNK_PATH, which only the widget build sets; it is served with max-age=0 today, where the previous step served it as immutable. That chunk is content-hashed, so it belongs with the other fingerprinted assets: the default output path now points at static/wasm/, which the existing /static/* rule already covers, and the widget build keeps overriding it with its own versioned path. A /*.module.wasm rule would work as well, since Cloudflare does support splat patterns of that shape, but letting one rule keep covering the whole fingerprinted bucket is the smaller change. robots.txt also gets its own comment, so the reason it must not be cached is visible in the file itself rather than only in the pull request. * docs(pages): say revalidate where the header means revalidate The comment above the robots.txt rule said "do not cache", but the value it describes is no-cache, must-revalidate. That does not forbid storing the response; it forbids reusing it without a successful revalidation, and only no-store forbids storing. The entry-point comment a few lines above already words it that way, so the two now match. The header value itself is unchanged.
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist