Skip to content

Replace ProminentButtonModifier with standard button rows and styles - #560

Merged
bradleymackey merged 1 commit into
mainfrom
native-standardization-buttons
Sep 14, 2026
Merged

bradleymackey merged 1 commit into
mainfrom
native-standardization-buttons

Conversation

@bradleymackey

Copy link
Copy Markdown
Member

Applies the button convention from #551 to the screens outside
Views/Backup, and deletes the modifier that made the old treatment
possible.

The problem

ProminentButtonModifier reimplemented .borderedProminent by hand:
.font(.headline) + a forced .foregroundStyle(.white) + padding(16/12)

  • .background(color) + RoundedRectangle(cornerRadius: 12). It applied
    .buttonStyle(.borderless) inside its own body, so the wrapped button
    never received a pressed state, and the hardcoded white label had no
    contrast guarantee against the tint it was placed on.

Six of its nine call sites also placed the button inside a Section
footer as a padded, centred pill — the exact pattern #551 called out and
removed from the backup screens.

What changed

Screen Change
SettingsDangerView "Delete All Data" becomes a red row in its own section; the error message becomes that section's footer
VaultTagDetailView "Delete Tag" becomes a red row
VaultItemDetailView "Unlock" and "Dismiss" move out of section footers into their own rows
EncryptedItemDetailView "Decrypt" moves out of the footer into its own section
VaultDetailEncryptionEditView "Encrypt" and "Remove Encryption" become plain rows
OTPCodeDetailView "Delete" moves out of the editing-actions footer into its own red section
SecureNoteDetailView "Delete" likewise
VaultAutofillConfigurationView "Continue" is a genuine standalone CTA outside any Form, so it becomes .buttonStyle(.borderedProminent) + .controlSize(.large)

Destructive emphasis is preserved the way #551 preserved it: a red
FormRow glyph tile plus red row text, matching BackupRestoreView's
Import & Override. Primary actions use the accent colour the same way
BackupKeyDecryptorView does.

The ProgressView in each loading: branch loses its .tint(.white),
which only existed because the spinner sat on a filled pill.

Toolbar Cancel buttons keep their explicit .foregroundStyle(.red).
Button(role: .cancel) does not render red in a toolbar, so dropping the
tint would have been a visual regression rather than a standardization.

Liquid Glass does not survive snapshot rendering

"Continue" was first written as .buttonStyle(.glassProminent). That
rendered VaultAutofillConfigurationView as a completely blank image
— not just the button, the whole hierarchy — dropping the reference from
151KB to 62KB.

Liquid Glass samples a backdrop through the render server, and the
.image snapshot strategy rasterises off-screen, so the effect resolves
to nothing and takes the rest of the frame with it.
Snapshotting.image(drawHierarchyInKeyWindow:) exists as a possible
escape hatch but changes the rendering path for every test, so it is not
something to adopt as a side effect of this PR.

.borderedProminent is used instead. On iOS 26 the system already draws
it as a capsule, which is the shape the hand-rolled 12pt rounded
rectangle was imitating.

Verification

Local, iPhone 18 Pro Max / iOS 27.0:

  • xcodebuild build-for-testing — TEST BUILD SUCCEEDED
  • Affected suites pass: OTPCodeDetailViewSnapshotTests,
    SecureNoteDetailViewSnapshotTests,
    VaultAutofillConfigurationViewSnapshotTests
  • 43 snapshots re-recorded (18 OTP detail, 24 secure note, 1 autofill)
    and visually reviewed
  • make format + make lint — clean

SettingsDangerView, VaultTagDetailView, EncryptedItemDetailView and
VaultDetailEncryptionEditView have view-model tests but no snapshot
coverage, so there was nothing to re-record for them.

⚠️ Automatic CI is still disabled (#548), so this is local verification only.

🤖 Generated with Claude Code

Applies the button convention from #551 to the screens outside
`Views/Backup`, and deletes the modifier that made the old treatment
possible.

## The problem

`ProminentButtonModifier` reimplemented `.borderedProminent` by hand:
`.font(.headline)` + a forced `.foregroundStyle(.white)` + `padding(16/12)`
+ `.background(color)` + `RoundedRectangle(cornerRadius: 12)`. It applied
`.buttonStyle(.borderless)` *inside* its own body, so the wrapped button
never received a pressed state, and the hardcoded white label had no
contrast guarantee against the tint it was placed on.

Six of its nine call sites also placed the button inside a `Section`
footer as a padded, centred pill — the exact pattern #551 called out and
removed from the backup screens.

## What changed

| Screen | Change |
| --- | --- |
| `SettingsDangerView` | "Delete All Data" becomes a red row in its own section; the error message becomes that section's footer |
| `VaultTagDetailView` | "Delete Tag" becomes a red row |
| `VaultItemDetailView` | "Unlock" and "Dismiss" move out of section footers into their own rows |
| `EncryptedItemDetailView` | "Decrypt" moves out of the footer into its own section |
| `VaultDetailEncryptionEditView` | "Encrypt" and "Remove Encryption" become plain rows |
| `OTPCodeDetailView` | "Delete" moves out of the editing-actions footer into its own red section |
| `SecureNoteDetailView` | "Delete" likewise |
| `VaultAutofillConfigurationView` | "Continue" is a genuine standalone CTA outside any `Form`, so it becomes `.buttonStyle(.borderedProminent)` + `.controlSize(.large)` |

Destructive emphasis is preserved the way #551 preserved it: a red
`FormRow` glyph tile plus red row text, matching `BackupRestoreView`'s
Import & Override. Primary actions use the accent colour the same way
`BackupKeyDecryptorView` does.

The `ProgressView` in each `loading:` branch loses its `.tint(.white)`,
which only existed because the spinner sat on a filled pill.

Toolbar *Cancel* buttons keep their explicit `.foregroundStyle(.red)`.
`Button(role: .cancel)` does not render red in a toolbar, so dropping the
tint would have been a visual regression rather than a standardization.

## Liquid Glass does not survive snapshot rendering

"Continue" was first written as `.buttonStyle(.glassProminent)`. That
rendered `VaultAutofillConfigurationView` as a **completely blank image**
— not just the button, the whole hierarchy — dropping the reference from
151KB to 62KB.

Liquid Glass samples a backdrop through the render server, and the
`.image` snapshot strategy rasterises off-screen, so the effect resolves
to nothing and takes the rest of the frame with it.
`Snapshotting.image(drawHierarchyInKeyWindow:)` exists as a possible
escape hatch but changes the rendering path for every test, so it is not
something to adopt as a side effect of this PR.

`.borderedProminent` is used instead. On iOS 26 the system already draws
it as a capsule, which is the shape the hand-rolled 12pt rounded
rectangle was imitating.

## Verification

Local, iPhone 18 Pro Max / iOS 27.0:

- `xcodebuild build-for-testing` — `TEST BUILD SUCCEEDED`
- Affected suites pass: `OTPCodeDetailViewSnapshotTests`,
  `SecureNoteDetailViewSnapshotTests`,
  `VaultAutofillConfigurationViewSnapshotTests`
- 43 snapshots re-recorded (18 OTP detail, 24 secure note, 1 autofill)
  and visually reviewed
- `make format` + `make lint` — clean

`SettingsDangerView`, `VaultTagDetailView`, `EncryptedItemDetailView` and
`VaultDetailEncryptionEditView` have view-model tests but no snapshot
coverage, so there was nothing to re-record for them.

⚠️ Automatic CI is still disabled (#548), so this is local verification only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bradleymackey
bradleymackey merged commit 2538fdf into main Sep 14, 2026
@bradleymackey
bradleymackey deleted the native-standardization-buttons branch September 14, 2026 16:48
bradleymackey added a commit that referenced this pull request Sep 15, 2026
## Problem

The Danger Zone — the single most destructive surface in the app — had
almost no coverage: one failure-path test on the view model
(`LightweightViewModelCoverageTests`) and no view test at all, despite
the screen being rebuilt in #560.

## Changes (test-only)

New `SettingsDangerViewModelTests`:

- `deleteEntireVault_success_callsDeleterAndClearsAutofillStore` — happy
path: store deleter invoked once, OTP autofill identities cleared,
`isDeleting` observed true in flight and false after (the deliberate
2-second completion delay makes this test ~2s wall clock; accepted
rather than refactoring the delay out pre-release).
-
`deleteEntireVault_deleterFailure_throwsPresentationErrorAndResetsState`
— deleter failure surfaces as `PresentationError`, autofill store
untouched.
- `deleteEntireVault_requiresAuthenticationBeforeDeleting` — denied
device auth means the deleter is never called (MANIFESTO C4: auth gates
the unattended-device threat).

New `SettingsDangerViewSnapshotTests` — first snapshots of the rebuilt
screen, light/dark × xSmall/medium/xxLarge.

No production code changes. (Noted for the release-findings list:
`deleteVault()` does not refresh the auto-backup payload hash, so the
newest auto-backup still describes the deleted vault — whether that is a
recovery safety net or a C6 problem is a design decision, not patched
here.)

Local verification: targeted suites passed on iPhone 18 Pro Max / iOS
27.0 (snapshots recorded, then clean pass).

⚠️ Automatic CI is still disabled (#548), so this is local verification
only.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
bradleymackey added a commit that referenced this pull request Sep 15, 2026
## Changes (test-only) — final PR of the pre-release audit series
(#561–#572)

Snapshot coverage for security-relevant screens rebuilt in #551–#560
that had no view tests:

- **`BackupKeyDecryptorViewSnapshotTests`** — initial state (light/dark
× 3 type sizes, in a NavigationStack so the Cancel toolbar renders) plus
a deterministic decrypt-failure state: the failure is produced by
actually running `attemptDecryption()` with the fast testing deriver and
an erroring decoder mock, not by faking view state.
- **`BackupImportFlowViewSnapshotTests`** — all three
`BackupImportContext` variants (empty vault / merge / override),
light/dark.
- **`VaultDetailEncryptionEditViewSnapshotTests`** — encryption-disabled
(full grid) and encryption-enabled variants.
- `SettingsDangerView` was covered in #565; `AutoBackupSettingsView` is
deliberately not given a standalone suite — it is already snapshotted
transitively through `BackupCreateView` (`BackupViewSnapshotTests`), and
its enabled/error states are only reachable through an async `.task`
handoff that would flake under synchronous snapshot rendering. Driving
those states needs a small initial-state injection refactor — left as
follow-up.

Release gate: full `CI_iOS` scheme (all 13 test targets, `iOSAllTests`
plan including the TSAN configuration) run locally on iPhone 18 Pro Max
/ iOS 27.0.

⚠️ Automatic CI is still disabled (#548), so this is local verification
only.

---

## Release findings — report-only (no code in this series)

The pre-release audit surfaced the following items that need **design
decisions**, not patches. Recorded here so they are not lost:

**MANIFESTO C7 gaps (protective defaults):**
- Clipboard paste TTL defaults to never-expire (`PasteTTL.default =
nil`) — copied OTPs/passwords sit on the pasteboard indefinitely unless
the user opts in to a TTL.
- No screenshot / app-switcher privacy protection anywhere (no
`privacySensitive()`, no capture detection, no cover view).
- Danger Zone full wipe has no confirmation dialog — one tap +
biometric.

**Design-level:**
- Backups export killphrase/search-passphrase salts+digests; anyone
holding the backup password can enumerate which items are
duress-protected (C5 tension).
- The killphrase/search-passphrase HMAC keys are device-local and not
exported, so a restore onto a new device silently disarms every
killphrase and permanently hides `.onlyPassphrase` items (rows exist,
digests unverifiable).
- No app-level lock / auto-lock; background purge clears only the backup
password from memory.
- Killphrase-triggered auto-backup + widget reload is an out-of-band
success signal for a hidden item's deletion (C2 tension).
- `payloadHash` and `lastBackupHash` live in plaintext UserDefaults —
mutation-time evidence (C6 tension).
- `deleteVault()` does not refresh the auto-backup hash, so the newest
auto-backup still describes the wiped vault (recovery safety net vs C6 —
decide).
- `DerivedEncryptionKey.debugDescription` prints raw key material as
hex; keychain replace (remove→store) is non-atomic;
killphrase/passphrase edit fields are plain `TextField` not
`SecureField`; the `vault://` HOTP-increment deep link is
unauthenticated; `Data.random` relies on `SystemRandomNumberGenerator`
(CSPRNG on Apple platforms, but unannotated as the app's sole randomness
source); no `protectedDataWillBecomeUnavailable` handling.

**Hygiene (non-blocking):** CI triggers commented out; CHANGELOG ~9
versions stale vs MARKETING_VERSION 2.0; hardcoded strings in rebuilt
screens bypass the string catalogs (app is currently English-only, so
cosmetic); stale scheme/test-plan references (`CI_iOS` scheme, orphan
`VaultUITests` scheme) and a stale snapshot directory;
`VaultBackup.xcstrings` not declared as a target resource; the keygen
speedtest CLI prints a derived key in hex; feed search reload has no
debounce/cancellation; `ForEach` identity built from
`Hasher().finalize()`; reorder persist failures are swallowed.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant