[romchornyi]

Merge order. dashwallet-ios tracks DashUIKit by branch (branch = master in project.pbxproj, revision pinned in Package.resolved), so any package update or pin bump on develop after this merges pulls it in. The app's grouped-transactions route no longer uses an in-sheet NavigationLink since dashpay/dashwallet-ios#1286 merged into develop, so this PR can merge; the app's DashUIKit pin moves after it.

Issue being fixed or feature implemented

Closes #24. BottomSheet still used three APIs that predate iOS 16: @Environment(\.presentationMode), a NavigationView around fillsHeight content, and .navigationBarHidden(true) on it.

What was done?

• The content sits directly in the sheet: no navigation container. The only thing the container did for current hosts was to give the content back the bottom safe area that the sheet ignored. The sheet now keeps its safe areas instead, so the content still stops above the home indicator and keyboard avoidance stays on. The background (the shape-style .background) still runs under the home indicator.
• The close button dismisses through @Environment(\.dismiss).
• The fillsHeight and natural-height paths are one view with the mode passed as values. They differed only in maxHeight, fixedSize and the height preference. Flipping fillsHeight no longer rebuilds the sheet and drops host state.
• The content no longer draws a second copy of the sheet background (it was there to paint over the NavigationView).
• The background doc no longer claims BottomSheet sets presentationBackground (only selfSizing → selfSizingSheet does).
• New preview: a scrolling fillsHeight sheet with a text field at the bottom.
• docs/navigation-and-containers.md describes the behaviour and how to keep an in-sheet route in host state (list kept alive under the detail, back button, VoiceOver escape).

The public API is unchanged.

How Has This Been Tested?

• swift build, swift test, and xcodebuild test -scheme DashUIKit on an iPhone 17 Pro / iOS 27 simulator: all green. conventions.sh: OK.
• A screenshot/probe harness (an SPM test package rendering in a real UIWindow with layer.render), run against origin/master (402db65) and this branch:
  • 14 rendered states, byte-identical PNGs: title + back, plain, custom background, natural height, content that ignores the safe area, content with its own background, scroll content; each in light and dark.
  • Frames of the content top and of a bottom text field, plus pixels in the body and in the 34 pt home-indicator strip: identical in light, dark and with a custom background.
  • Close button: dismisses a SwiftUI .sheet on both master and this branch. For a UIKit presented hosting controller, the harness cannot observe a dismiss on master or on this branch, so it gives no signal there. @Environment(\.dismiss) already closes UIKit-presented BottomSheets in production (dashwallet-ios HomeViewController+Shortcuts → ShortcutSelectionView).
  • A state-driven in-sheet route (list kept alive under the detail, back button) behaves the same on both. A NavigationLink push inside the sheet works on master and does not on this branch — the expected breaking change.
• Not covered by the harness: the keyboard (it does not rise in this harness on master either), and a BottomSheet embedded as a child view controller (dashwallet-ios RequestAmountHostingController). Tapping X there was checked by hand in the app (refactor(ui): open a grouped transaction's details inside the sheet dashwallet-ios#1286); it needs one more check after the app bumps DashUIKit past this change.

Before / After

All 14 harness states are byte-identical; three of them:

	Before	After
Light, title + back
Dark, title + back
Dark, custom background
Light, natural height

• No visual change (refactor, tests, docs). Say why in "What was done?".

Breaking Changes

Behavioural, not in the API: content inside a fillsHeight BottomSheet no longer has a navigation container. A NavigationLink inside it no longer pushes, and a UIKit view controller wrapped in the content has no navigationController. A host that moves between screens inside the sheet keeps the route in its own state and drives the header's back button (see the docs). In dashwallet-ios the only such host is the grouped-transactions screen, which the companion app change moves first.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Filled bottom sheets now retain keyboard avoidance, and natural-height sheets measure their content for sizing.
  • Closing a sheet uses SwiftUI’s dismissal behavior; interactive dismissal remains controlled by its existing setting.
• Behavior Changes
  • Bottom-sheet content no longer includes a navigation container. Navigation links will not push destinations within the sheet, and embedded UIKit controllers won’t have a navigation controller.
• Documentation
  • Updated guidance on managing sheet navigation, VoiceOver announcements, and focus restoration.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/DashUIKit
• Branch: refactor/bottomsheet-navigation
• File: .coderabbit.yaml
• Commit: cce7bb9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2b9ce117-075a-4feb-b243-bf38202834ec

Commits

Reviewing files that changed from the base of the PR and between 3154466 and cce7bb9.

Files selected for processing: 2

• Sources/DashUIKit/Components/BottomSheet/BottomSheet.swift
• docs/navigation-and-containers.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

BottomSheet now renders filled and natural-height content without a NavigationView. It uses the dismiss environment action for close-button dismissal, applies height measurement only to natural-height sheets, and updates documentation to describe navigation and accessibility behavior.

Changes: BottomSheet presentation

Layer / File(s)	Summary
Sheet rendering, dismissal, and guidance Sources/DashUIKit/Components/BottomSheet/BottomSheet.swift, docs/navigation-and-containers.md	Both sizing modes use shared sheet content. Natural-height sheets retain vertical fixed sizing and height measurement; filled sheets do not. The close button uses dismiss. The preview and documentation describe the updated sheet layout and navigation behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Possibly related PRs

• dashpay/DashUIKit#15: It set the iOS deployment baseline that precedes removal of the remaining legacy BottomSheet APIs.
• dashpay/DashUIKit#47: It changes BottomSheet sizing behavior, including paths refactored in this PR.
• dashpay/DashUIKit#40: Its BottomSheet snapshot tests cover sizing and dismissal behavior changed here.

Suggested reviewers: jeanpierreroma

Merge Risk: Minimal · up to cce7b

No actionable merge-blocking issue remains. The navigation-container removal is documented, including host responsibilities for routing and accessibility.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main change: removing the navigation container from BottomSheet.
Linked Issues check		Issue #24 coding requirements are implemented. BottomSheet uses @Environment(\\.dismiss), removes the NavigationView and navigationBarHidden usage, and unifies the filled and natural-height pat…
Out of Scope Changes check		The changed source supports Issue #24 by removing obsolete SwiftUI APIs, preserving sheet sizing and dismissal behavior, and adding the related preview. The documentation explains the resulting naviga…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Linked Issues check

Explanation

Issue #24 coding requirements are implemented. BottomSheet uses @Environment(\.dismiss), removes the NavigationView and navigationBarHidden usage, and unifies the filled and natural-height paths. BottomSheetDismissalAction.perform still invokes onClose first and otherwise calls dismiss() only when dismissal is enabled. The documentation explains the new container behavior and host-managed routes. The PR reports successful build and test runs. The reported missing keyboard visual check is a manual validation item, not a coding requirement.

Full details: Out of Scope Changes check

Explanation

The changed source supports Issue #24 by removing obsolete SwiftUI APIs, preserving sheet sizing and dismissal behavior, and adding the related preview. The documentation explains the resulting navigation behavior and migration pattern. No unrelated change is established.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[romchornyi-pitcrew]

✅ Review posted for commit cce7bb9 · triage: normal · approve · took 7 min

[romchornyi-pitcrew]

✅ no blockers · no majors · 6 non-blocking recommendations — reviewed at cce7bb9b2e

Level	Verdict
1 · Idea	✅ valid	Closes #24: presentationMode, NavigationView and .navigationBarHidden predate the iOS 18 floor. Dropping the container (instead of swapping in a NavigationStack) removes the only reason it existed — handing the bottom safe area back to the content — and the sheet now keeps its safe areas itself
2 · Architecture	✅ right layer	The container and the close button live in DashUIKit; in-sheet routing moves to host state, which the header's showBackButton / onBackButtonPressed already supported
3 · Completeness	✅ complete	Every fillsHeight BottomSheet host in dashwallet-ios develop (2ba855841f, 28 call sites) was read: none uses NavigationLink, navigationDestination, .searchable, .toolbar, .navigationTitle or a wrapped controller's navigationController inside the sheet. The grouped-transactions route is already off NavigationLink (dashpay/dashwallet-ios#1286), so the merge-order note holds
4 · Code	✅ clean	Natural-height measurement is unchanged (same fixedSize → GeometryReader → preference order, bottom safe area still not ignored); .background(_:ignoresSafeAreaEdges:) is the overload .background(Color) already resolved to; fixedSize(vertical: false) is a no-op; dismiss and presentationMode resolve to the same presentation from the header. CI is green

6 non-blocking recommendations

1. The documented route leaves VoiceOver escape on the header closing the sheet — docs/navigation-and-containers.md#L166 — the snippet puts .accessibilityAction(.escape) on the detail only. With focus on the library-owned header (Back, title, Close) the escape is not inside that subtree, so it reaches the presentation and closes the whole sheet instead of going back; a UIKit-wrapped detail never sees the SwiftUI action at all. The app's own host puts the action on the BottomSheet itself and adds a UIKit escape for the wrapped detail (dashwallet-ios HomeView.swift L1193–L1194, L1239–L1241). Moving the snippet's action to the sheet — or having BottomSheet map escape to onBackButtonPressed while showBackButton is true — would match it. (fall-through not run on a device)
2. The docs name only part of what the container provided — docs/navigation-and-containers.md#L141-L145, BottomSheet.swift#L33-L35 — besides NavigationLink and navigationController, .searchable, .navigationTitle, navigation-bar .toolbar items and navigationDestination also stop working inside a fillsHeight sheet — or, when a host's own NavigationStack encloses the BottomSheet, now attach to that outer stack (a NavigationLink then pushes there), which the hidden inner NavigationView used to swallow. Naming them, plus the other migration (the content wraps its own NavigationStack with the bar hidden), would save the next host a surprise. Separately, the background doc (#L37-L41) says the fill runs under the home indicator unconditionally; for fillsHeight: false without selfSizingSheet (a UIKit detent) that strip is the presentation background, not background.
3. No snapshot covers the rewritten fillsHeight path — BottomSheetSnapshotTests.swift — all BottomSheet snapshot tests use fillsHeight: false; the default path this PR changes is pinned only by the out-of-repo harness. The repo CLAUDE.md asks for a snapshot per visually meaningful state; one fillsHeight state (title + back, custom background) would pin content and background layout. The new "Text Field" preview (BottomSheet.swift#L526-L552) renders the sheet as the canvas root, so it shows neither the sheet's safe areas nor the keyboard it comments on; wrapping it in a .sheet with a detent, as the other sheet previews do, would exercise the real path.
4. Content that ignores every safe-area region and the keyboard — BottomSheet.swift#L78-L79 — content with a bare .ignoresSafeArea() (regions .all, so .keyboard too) now sits directly under the sheet's safe areas; under the old NavigationView the UIKit boundary may have kept a bottom field above the keyboard. No current app host combines this with a text field, and the PR's harness could not raise a keyboard. (not verified)
5. Close button in an embedded hosting controller — BottomSheet.swift#L155-L160 — the PR already lists it: dashwallet-ios RequestAmountHostingController embeds the sheet as a child controller with no onClose, so X relies on dismiss reaching the presenter. Worth the planned check right after the app's DashUIKit pin moves past this change, together with TxDetailViewController's UIKit-presented sheets (explorer selection, raw transaction).
6. In-sheet routing has no library primitive — docs/navigation-and-containers.md#L146-L170 — every host now re-implements the ZStack, hit-testing, accessibilityHidden, escape and focus wiring, and the snippet and the app's InSheetPushStack already differ (rec. 1). A small DashUIKit container for it would keep the pieces in one place; fine as a follow-up.

Review details

• Triage: normal (a shared container's behaviour; refactor! with no public API change).
• Levels 1–3 inline against the PR description, issue #24 and BottomSheet.swift at the head and master. Level 3 included an audit of every BottomSheet construction in dashwallet-ios develop (2ba855841f) for navigation-container dependencies, UIKit presentation and child embedding, bottom-inset and keyboard behaviour.
• Level 4: the general lane (/code-review high) and a UI/compatibility lane (layout equivalence of both modes, selfSizing measurement, identity, dismiss semantics, macOS / iOS 18, the repo CLAUDE.md, docs and snippet correctness).
• Verifier: recs 2, 3 and 6 were checked against the code; rec 1 against the code and the app's own escape handling; recs 4 and 5 need a running app and are marked accordingly. Kept out: the PR body's "flipping fillsHeight no longer rebuilds the sheet" is true of the component, but the one app host that flips it (gift-card sheet) branches itself, so the benefit does not reach the app yet — description only.

🤖 Reviewed with Claude Code