Skip to content

fix: persist default-account modal flag across sessions #3818

Merged
grimen merged 5 commits into
mainfrom
fix--persist-has-prompted-set-default-account
Jun 8, 2026
Merged

fix: persist default-account modal flag across sessions #3818
grimen merged 5 commits into
mainfrom
fix--persist-has-prompted-set-default-account

Conversation

@esaugomez31

@esaugomez31 esaugomez31 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3811 — the "Select default account" modal re-appearing on every Receive tap.

The flag tracking whether the modal has already been shown lived in the Apollo client-only cache (hasPromptedSetDefaultAccount). Its restore was token-gated in PR #3769 and useEffectiveAuthToken() resolves asynchronously, so on cold start the restore is often skipped and the flag defaults to false — the modal re-appears.

This PR moves the flag out of Apollo cache into persistentState, keyed by accountId. Same persistence pattern used by PR #3795 (theme preference).

Why persistentState

  • Independent of Apollo client lifecycle and token-gating
  • Loaded synchronously on app boot via PersistentStateProvider
  • Per-account isolation comes for free (defaultAccountModalShownByAccountId[id]), which also fixes the secondary issue where the choice was lost on account switches

Changes

New

  • app/store/persistent-state/default-account-modal-shown.tsgetDefaultAccountModalShown / markDefaultAccountModalShown
  • app/hooks/use-default-account-modal-shown.tsuseDefaultAccountModalShown
  • __tests__/store/persistent-state/default-account-modal-shown.spec.ts — 9 specs covering per-account isolation, fallback, immutability

Schema migration

  • PersistentState_13 adds defaultAccountModalShownByAccountId?: Record<string, boolean>
  • migrate12ToCurrent → bumps to 13, migrate13ToCurrent is identity

Removed (Apollo client-only field)

  • hasPromptedSetDefaultAccount from local-schema.gql
  • Type policy from cache.ts
  • setHasPromptedSetDefaultAccount writer + query from client-only-query.ts
  • Regenerated generated.ts

Consumers migrated

  • home-screen.tsx — reads defaultAccountModalShown from the new hook
  • set-default-account-modal.tsx — calls markDefaultAccountModalShown() instead of writing the Apollo cache

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@esaugomez31 esaugomez31 changed the title fix(home): persist default-account modal shown flag across sessions and account switches fix: persist default-account modal flag across sessions Jun 3, 2026
@esaugomez31 esaugomez31 self-assigned this Jun 3, 2026
@esaugomez31 esaugomez31 marked this pull request as ready for review June 3, 2026 18:12
@esaugomez31 esaugomez31 requested a review from grimen June 3, 2026 18:15

@grimen grimen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — bugs + test coverage

Critical

None.


Important

  • I1 — Add the upgrade tradeoff to the PR description.
    app/store/persistent-state/state-migrations.ts:106-107
    Users who already dismissed the modal under the Apollo mechanism will see it one more time after updating — the old flag lived in Apollo cache and is structurally unavailable to migrate12ToCurrent. One-time, then it sticks. QA should expect it.

  • I2 — Backdrop/close-icon dismiss does not mark the flag — pre-existing, unchanged.
    app/components/set-default-account-modal/set-default-account-modal.tsx:150-159
    Only the BTC/USD selection handlers mark it; dismissing via backdrop re-prompts later. Identical on main, so not a regression — recorded so the semantics are a decision, not an accident.


Design

SOLID violations

  • None.

Incomplete adapter pattern

  • D1 — resolveAccountKey copy-pasted; the per-account pattern's 5th use has no shared key primitive.
    app/store/persistent-state/theme-preference.ts:7-8 + app/store/persistent-state/default-account-modal-shown.ts:5-6
    Character-for-character identical (activeAccountId ?? DefaultAccountId.Custodial), and it encodes the write-key-must-equal-read-key invariant this PR depends on — one definition means it can't silently diverge between features. Extract to a shared app/store/persistent-state/account-key.ts before the 6th copy. Keep self-custodial-language.ts:5-9's resolver separate — different semantics (excludes custodial).

High cyclomatic complexity

  • None.

Code smells

  • D2 — Duplicated markDefaultAccountModalShown(); toggleModal() sequence in both handlers.
    set-default-account-modal.tsx:80,103
    Extract closeAsAcknowledged() — dropping the mark from one branch silently reintroduces the bug for that choice path.

Naming smells

  • D3 — markDefaultAccountModalShown names two different things.
    app/store/persistent-state/default-account-modal-shown.ts:13 + app/hooks/use-default-account-modal-shown.ts:23
    A pure transformer (state) => state in the store and a void imperative callback returned by the hook — the hook imports one and exports the other under the identical name, so auto-import will pick the wrong one. Also breaks the sibling with* convention for pure transformers (withThemePreference, withSelfCustodialLanguage). Rename the store fn to withDefaultAccountModalShown — fixes both.
  • UseDefaultAccountModalShownReturn vs siblings' EffectiveThemeReturn / EffectiveLanguageReturn (no Use prefix). Cosmetic.
  • Stale spec title at __tests__/store/persistent-state/state-migrations.spec.ts:113 — says "preserves schema 11 …" over a schemaVersion: 12 input. Rename.

Test coverage

Tier 1 — production code with no test file

  • app/hooks/use-default-account-modal-shown.ts (25 LOC, new) — contains the only non-trivial wiring in the feature: the functional updater updateState((prev) => prev && mark(prev)). Exact copy-able precedent exists: __tests__/hooks/use-effective-language.spec.ts:158-166 (mock context → capture updater from mock.calls[0][0] → apply to fake prev → assert).

Tier 2 — tests that pass while a regression could ship invisibly

  • __tests__/store/persistent-state/state-migrations.spec.ts — all 18 changed lines are mechanical 12→13 find-replace; defaultAccountModalShownByAccountId and themeByAccountId appear nowhere in the spec. No behavioral spec for the new v12→v13 step or the v13 identity — a future migration step that forgets to spread a field ships invisibly.
  • __tests__/store/persistent-state/default-account-modal-shown.spec.ts — optional gaps: explicit activeAccountId === DefaultAccountId.Custodial case; empty-string accountId ("" ?? x keeps ""). Low priority.

Tier 3 — recommended regression tests per finding

  • Hook updater semantics (→ Tier 1) — __tests__/hooks/use-default-account-modal-shown.spec.ts: assert markDefaultAccountModalShown calls updateState with a functional updater (regressing to a direct-value update would clobber concurrent writes); apply the captured updater to a fake prev and assert the resulting map; assert the updater returns falsy (no write) when prev is undefined.
  • Migration field preservation (→ Tier 2) — "migrates a v12 state to v13 preserving themeByAccountId and self-custodial maps"; "v13 identity migration preserves defaultAccountModalShownByAccountId untouched".
  • Both modal handlers mark the flag (→ D2) — mock the hook in a set-default-account-modal component spec and assert markDefaultAccountModalShown fires on both the keep-current and set-new paths — or skip the spec and rely on the closeAsAcknowledged() extraction (one of the two suffices).

@grimen grimen marked this pull request as draft June 5, 2026 13:52
@grimen grimen marked this pull request as ready for review June 5, 2026 13:55
@esaugomez31 esaugomez31 force-pushed the fix--persist-has-prompted-set-default-account branch from 8eac43b to 0db18c8 Compare June 5, 2026 17:21
@esaugomez31

Copy link
Copy Markdown
Collaborator Author

@grimen

Important

I1 — add the upgrade tradeoff to the PR description
Done! Added to the description: users who dismissed the modal under the old Apollo mechanism will see it one more time after updating (the old flag lived in Apollo cache, unreachable from migrate12ToCurrent), then it sticks. QA should expect the one-time re-prompt.

I2 — backdrop/close-icon dismiss does not mark the flag (pre-existing)
Kept as-is by decision. Identical to main, so out of scope for this fix — only the BTC/USD selection handlers acknowledge the modal; backdrop/close intentionally re-prompts later. Recording it as a deliberate semantic rather than folding an unrelated behavior change into this PR.

Design

D1 — resolveAccountKey copy-pasted; no shared key primitive
Done! Extracted to app/store/persistent-state/account-key.ts; theme-preference.ts and default-account-modal-shown.ts now both import the single definition. Left self-custodial-language.ts's resolver separate (different semantics — excludes custodial).

D2 — duplicated mark(); toggle() sequence in both handlers
Done! Extracted closeAsAcknowledged(); both the keep-current and set-new paths call it, so dropping the mark from one branch can't silently reintroduce the bug.

D3 — markDefaultAccountModalShown names two different things
Done! Renamed the store transformer to withDefaultAccountModalShown, matching the with* sibling convention (withThemePreference, withSelfCustodialLanguage); the hook keeps the imperative markDefaultAccountModalShown callback, so auto-import can no longer pick the wrong one.

  • Return type renamed UseDefaultAccountModalShownReturnDefaultAccountModalShownReturn for sibling parity.
  • Fixed the stale spec title that said "preserves schema 11" over a schemaVersion: 12 input.

Test coverage

Tier 1 — use-default-account-modal-shown.ts
Done! Added __tests__/hooks/use-default-account-modal-shown.spec.ts following the use-effective-language precedent: asserts updateState is called with a functional updater, applies the captured updater to a fake prev and asserts the resulting map, and asserts the updater returns falsy (no write) when prev is undefined.

Tier 2 — migration field preservation
Done! Added a v12→v13 test preserving themeByAccountId and the self-custodial maps, plus a v13-identity test asserting defaultAccountModalShownByAccountId passes through untouched.

Tier 3 — per-finding regression
Done! The hook updater semantics and migration field-preservation are covered above; the "both handlers mark the flag" case is covered by construction via the closeAsAcknowledged() extraction (D2), which you noted suffices over a component spec.

@esaugomez31 esaugomez31 requested a review from grimen June 5, 2026 17:28
@grimen grimen merged commit 48b3942 into main Jun 8, 2026
7 of 9 checks passed
@grimen grimen deleted the fix--persist-has-prompted-set-default-account branch June 8, 2026 08:40
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.

fix(receive): Default account modal re-appears on every "Receive" tap — choice not persisted (regression since 2.4.46)

2 participants