Replace ProminentButtonModifier with standard button rows and styles - #560
Merged
Merged
Conversation
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
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>
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
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.
Applies the button convention from #551 to the screens outside
Views/Backup, and deletes the modifier that made the old treatmentpossible.
The problem
ProminentButtonModifierreimplemented.borderedProminentby 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 buttonnever 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
Sectionfooter as a padded, centred pill — the exact pattern #551 called out and
removed from the backup screens.
What changed
SettingsDangerViewVaultTagDetailViewVaultItemDetailViewEncryptedItemDetailViewVaultDetailEncryptionEditViewOTPCodeDetailViewSecureNoteDetailViewVaultAutofillConfigurationViewForm, so it becomes.buttonStyle(.borderedProminent)+.controlSize(.large)Destructive emphasis is preserved the way #551 preserved it: a red
FormRowglyph tile plus red row text, matchingBackupRestoreView'sImport & Override. Primary actions use the accent colour the same way
BackupKeyDecryptorViewdoes.The
ProgressViewin eachloading: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 thetint would have been a visual regression rather than a standardization.
Liquid Glass does not survive snapshot rendering
"Continue" was first written as
.buttonStyle(.glassProminent). Thatrendered
VaultAutofillConfigurationViewas 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
.imagesnapshot strategy rasterises off-screen, so the effect resolvesto nothing and takes the rest of the frame with it.
Snapshotting.image(drawHierarchyInKeyWindow:)exists as a possibleescape hatch but changes the rendering path for every test, so it is not
something to adopt as a side effect of this PR.
.borderedProminentis used instead. On iOS 26 the system already drawsit 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 SUCCEEDEDOTPCodeDetailViewSnapshotTests,SecureNoteDetailViewSnapshotTests,VaultAutofillConfigurationViewSnapshotTestsand visually reviewed
make format+make lint— cleanSettingsDangerView,VaultTagDetailView,EncryptedItemDetailViewandVaultDetailEncryptionEditViewhave view-model tests but no snapshotcoverage, so there was nothing to re-record for them.
🤖 Generated with Claude Code