Skip to content

test(wallet): add RTL assertions for wallet_selector_modal (#232) - #298

Closed
MAN7A-afk wants to merge 4 commits into
Goldii-locks:mainfrom
MAN7A-afk:feat/232-wallet-selector-modal-tests
Closed

test(wallet): add RTL assertions for wallet_selector_modal (#232)#298
MAN7A-afk wants to merge 4 commits into
Goldii-locks:mainfrom
MAN7A-afk:feat/232-wallet-selector-modal-tests

Conversation

@MAN7A-afk

Copy link
Copy Markdown

Description:

Closes #232

Adds React Testing Library assertions for WalletSelectorModal covering trigger actions under mocked wallet state, beyond the baseline coverage already in place.

New coverage added:

  • Disabled trigger button does not fire onConnect
  • Wallet picker combobox correctly reflects the selectedWalletId prop
  • Dialog panel exposes aria-labelledby, correctly pointing at the modal title
  • Modal state (multi-sig toggle, XDR parse error, structure preview) resets cleanly on close/reopen
  • XDR input validate button is disabled when empty; error and preview states clear on new input
  • Build-plan button stays disabled when no structure preview exists, even with signer input present
  • onMultiSigPlanReady is not called when the build-plan action has no signers
  • onConnect and onSelectWallet are asserted to fire exactly as expected under mocked wallet actions

Testing:

  • __tests__/wallet_selector_modal.test.tsx: 35/35 passing (20 existing + 15 new)
  • Full existing test suite: all passing, no regressions
  • Typecheck: no new errors introduced (pre-existing next/* module resolution errors unrelated to this change)

No source files were modified — this is test-only.

- Add DEFAULT_SIGNING_TIMEOUT_MS = 60_000 configurable constant
- Add TxSignRequest interface, TxSignatureTimeoutError class,
  clearTxSensitiveMemory helper, and signTxWithTimeout function to
  app/lib/transactions.ts, matching the setTimeout + Promise.race
  pattern used by all other connector helpers in the codebase
- Wire signTxWithTimeout into submitContractTransaction via optional
  signingTimeoutMs param (defaults to the constant); on timeout the
  operation is aborted and any sensitive payload memory is zeroed
- Add __tests__/transactions_timeout.test.ts with 21 test cases
  covering: timeout fires, memory cleared, successful signing flow,
  pre-deadline no-early-fire, timer cleanup, error propagation,
  default constant, TxSignatureTimeoutError class, and
  clearTxSensitiveMemory helper
@drips-wave

drips-wave Bot commented Aug 26, 2026

Copy link
Copy Markdown

@MAN7A-afk Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@MAN7A-afk
MAN7A-afk force-pushed the feat/232-wallet-selector-modal-tests branch from fddba24 to 4e942aa Compare August 26, 2026 18:26
@godamongstmen897

Copy link
Copy Markdown
Contributor

Thanks @MAN7A-afk — your #296, #297 and #301 all landed (GitHub closed #296/#297 as merged once their commit became reachable through #301, so they're credited). This one I'm holding, because it can't stand on its own against current main.

This PR doesn't include the component its tests need. Its diff against its own base touches only __tests__/wallet_selector_modal.test.tsxapp/components/WalletSelectorModal.tsx is unchanged. The suite drives a self-managing modal:

function openModal() {
  fireEvent.click(screen.getByTestId("wallet-selector-modal-trigger"));
}
render(<WalletSelectorModal {...defaultProps({ selectedWalletId: "albedo" })} />);

That design — internal isOpen state plus a trigger button — came from the shared branch behind #296/#297. But the version on main is the controlled one: isOpen and onClose are props, there's no trigger, and wallet-selector-modal-trigger appears nowhere in app/. Merging this PR gives 40 TypeScript errors, starting with Cannot find name 'defaultProps', because the two versions of the test file splice together and the helper definitions are lost.

Why the controlled design won: #355 (design tokens) and the availability/rejection suites on main are written against isOpen/onClose, and they pass. The self-managing version was superseded rather than rejected — merge order, not a judgement on the approach.

The 35 assertions here are genuinely good and cover ground main doesn't (multi-sig toggle, XDR validation gate, assembly plan, modal reset on close). Nothing is wrong with them except the component they assume.

Suggested path: rebase on main and pick one:

  • Adapt the suite to the controlled component — pass isOpen directly and drop openModal(), keeping every assertion. This is the smaller change.
  • Or bring the self-managing wrapper back as a deliberate change in this PR, alongside updates to the design-token and availability suites that currently rely on isOpen.

Also worth knowing for next time: #296 and #297 were opened with maintainer edits disabled (#296 from your fork's default branch), so I couldn't refresh their branches or re-run CI against current main. Opening from a topic branch with "Allow edits by maintainers" ticked lets a maintainer keep them current.

@godamongstmen897

Copy link
Copy Markdown
Contributor

@MAN7A-afk — closing this one, and I want to be straight about why, because the problem isn't the code you wrote.

The component these tests target was never merged. I re-measured against current main: of the 13 data-testids the suite drives, 10 don't exist anywhere in app/:

test id on main?
wallet-selector-modal, -close, -content yes
wallet-selector-modal-trigger no
multisig-toggle, multisig-panel, multisig-error no
xdr-input, validate-xdr-btn, tx-structure-preview no
signer-input, build-plan-btn, connect-btn no

Of the 35 it(...) blocks, exactly one runs against the component that's actually there. The rest need a multi-signature XDR assembly surface — an XDR field, a validate button, a structure preview, a signer list, a build-plan button — inside the wallet selector.

That surface came from the shared branch behind #296/#297. Those merged, but the self-managing modal they carried was superseded by the controlled isOpen/onClose version that #355 and the availability/rejection suites are written against. Merge order, not a verdict on the approach — but it means the thing your tests describe doesn't exist and isn't coming back in that shape.

Why I'm not building it to make this mergeable: app/components/TransactionSignerPanel.tsx already does XDR parsing and multi-sig signing, using the useFreighterMultiSigAssembly / useLedgerMultiSigAssembly / useAlbedoMultiSigAssembly hooks. Adding a second signing surface inside the wallet selector would mean two independent XDR paths to keep in step. That's an architecture decision, and it shouldn't be made as a side effect of landing a test file.

What was genuinely good here: the assertions themselves. The multi-sig toggle behaviour, the XDR validation gate, the assembly plan, and especially the modal-state-reset-on-close cases cover ground nothing else does. If you want to bring that rigour to the code that is on main, TransactionSignerPanel is the natural home for most of it and is thinly tested by comparison — I'd review that gladly.

Your #296, #297 and #301 all landed and are credited. Sorry this one ran aground on merge ordering rather than on anything you did.

One practical note for next time: #296 was opened from your fork's default branch with maintainer edits disabled, so I couldn't refresh it against main or re-run CI. A topic branch with "Allow edits by maintainers" ticked lets a maintainer keep a PR current instead of watching it drift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write React Testing Library assertions for wallet_selector_modal

2 participants