Skip to content

feat: add Settings as a fourth bottom-navigation destination - #807

Open
BrianSeong99 wants to merge 2 commits into
mainfrom
brian/settings-bottom-navigation
Open

feat: add Settings as a fourth bottom-navigation destination#807
BrianSeong99 wants to merge 2 commits into
mainfrom
brian/settings-bottom-navigation

Conversation

@BrianSeong99

Copy link
Copy Markdown
Collaborator

Research participants rarely found Settings from the Home balance-card gear, and several went hunting for it under Activity. /settings sat outside the persistent tab layout — PageRouter wrapped it in FullScreenPage, which calls useHideNavbarWhileOpen() unconditionally and therefore hid the bottom nav on the very screen you had navigated to.

Settings is now the fourth bottom-nav destination, beside Home, Explore and Activity.

Two findings that changed the shape of this

The native mobile navbar is dead code. The JS↔native bridge (showNativeNavbar, setNavbarSecondaryRow, nativeNavbarTap, …) was deleted by 5fd95811 "v0 UI Revamp (#248)". There are zero call sites in src/ on both main and next, and src/main.css:335 says so outright: "Wallet navigation is React-only: BottomNav is mounted inside TabLayout." The Swift/Java overlay is intact but unreachable. CLAUDE.md:172, AGENTS.md and the issue all still describe it as live — they are stale.

So the React BottomNav is the mobile navbar, and changing it aligns extension, mobile and desktop by construction rather than by keeping three lists in sync. The issue's "the native mobile navbar contains the same four primary destinations as the React footer" is satisfied because there is only one navbar.

The "mobile secondary quick-action row" no longer exists. It was the native Send/Receive/Settings row, deleted with the bridge. The live duplicate was TabHeader's gear, which rendered on Activity and Explore — exactly the two screens that now carry a Settings tab in the same bar. That gear is gone.

What

Routing — one new exact-matched route puts the Settings root in the tab shell, above the existing /settings/:tabSlug?. Sub-pages keep their FullScreenPage drill-in, so opening a section and backing out is byte-for-byte the behaviour it was, and so is hardware back from either place (MobileBackBridge already listed /settings in TAB_PAGES and is untouched).

The tabTAB_ROUTES gains settings, activeTabFromPath matches on the path segment so /settings/<slug> keeps the tab lit, and the item reuses the already-wired IconName.Settings. Tab labels are now localized: settings, home, explore and activity all already exist in en.json, so no new keys and nothing for the DeepL job to translate.

Header — the Settings root wears the same TabHeader as Activity and Explore: a plain title, no back chevron, because a tab destination is not something you back out of. Its sub-pages keep NavigationHeader, whose chevron is their only way out. This also retires dead space — Settings.tsx already carried pb-22 "so the last menu item can scroll above the React BottomNav", for a nav that did not render there.

Balance-card icon — the account-options button on the Home balance card was a gear. It opens account options, not Settings, and a gear sitting a thumb away from a Settings tab reads as the tab. It is now a pencil (new IconName.Edit).

Fit, not restyle — the pill's px-13.5 gap-8 was tuned for three items; TabLayout.tsx:241-244 already noted its min-content was ~367px against a 375px viewport. A fourth item pushed that to ~470px and overflowed every phone. Reduced to px-4 gap-2. Colours, radius, shadow, typography and structure are untouched — #803 still owns the visual treatment.

One latent native bug fixedNavbarView.java:345 hardcodes navStack.setWeightSum(3) while rebuildMainRow() adds each button at weight 1f and never recomputes it, so a fourth button would claim 1/3 of the width and clip off the right edge. Now setWeightSum(items.size()), mirroring what the secondary row already does correctly at :929. Dormant code today; correct whenever #803 revives it.

Testing

  • 8557 passing, 4 skipped, 0 failing (yarn test, full suite). yarn ts, yarn lint:i18n, yarn lint:e2e clean; ESLint clean on every changed file.
  • New/updated coverage: /settings and /settings/<slug> → settings tab active; tapping the tab navigates and fires exactly one hapticSelection; re-tapping on-route is a silent no-op; tapping from a sub-page returns to the root; tab order and the localization of all four labels; four-destination rendering and single-active-tab in BottomNav; /settings inside TabLayout and sub-pages outside it in PageRouter; TabHeader renders no settings-labelled control and no empty actions row; the balance card renders the edit glyph.
  • TabLayout.test.tsx:185 asserted /settingshome tab. That test encoded the old behaviour and is inverted, not deleted. Same for the three Settings.test.tsx cases that asserted the root's NavigationHeader and its back-to-home chevron — the back-button test is replaced by a positive assertion that the root has no back affordance and never calls navigate.
  • Verified on the iPhone 17 simulator against testnet: Settings reachable in one tap, active on /settings, list scrolls above the pill, Home ↔ Settings switching correct, four tabs fit without overflow.

Reviewers

  • Base branch. This targets main, deliberately. Most work here lands on next first, but next currently pins @miden-sdk/miden-sdk 0.16.0-rc.5 (miden-client 0.16.0-rc.3) and testnet rejects that client at the handshake — "accept header validation failed … (client version: 0.16.0-rc.3)" — so nothing on next can create a wallet locally to verify against. It will need porting to next alongside the rest.
  • TabHeader is imported from its module, not the components/ui barrel. The barrel pulls in siblings that call lib/platform predicates at module scope, and Settings.test.tsx mocks that module with only isMobile; going through the barrel took the whole suite down on a TDZ error. Fixing the mock instead meant editing an unrelated suite's mocking strategy, so I took the smaller route. Happy to flip it if you'd rather fix the mock.
  • Not verified: Android and desktop. No emulator or Tauri run in this pass. The Android change is a one-line weightSum fix in code nothing currently calls, and neither platform has navbar tests.
  • Unrelated bug spotted while verifying, not fixed here: Welcome.tsx's confirmation handler swallows registration failures for the Create + passcode path — the catch only surfaces an error for Import+Guardian and hardware-only-biometric, so any other failure leaves a dead "Open wallet" button with no message. Worth its own issue.

closes #802

@canoplos

Copy link
Copy Markdown

This is clean and well-thought-out. A few things I checked:

  • The BottomNav spacing change (gap-8 px-13.5gap-2 px-4) is justified — the old values did push min-content past a 375px viewport once a fourth destination was added, and the comment scoping the visual treatment to Add a scroll-responsive liquid-glass navbar #803 keeps this PR appropriately narrow.
  • Preserving the /settings/<slug> FullScreenPage drill-in while only making the /settings root a tab destination is the right call — it keeps back-behaviour and history depth unchanged, which is exactly where a nav change like this could regress.
  • Nice bonus that the existing hardcoded 'Home' / 'Explore' / 'Activity' labels got moved to t(...) alongside the new t('settings'). I confirmed the settings / home / explore / activity keys exist across all public/_locales/*/messages.json (en, de, ja, ko, pl), so no raw-key fallback on non-EN locales.
  • Test coverage is thorough: tab mapping, sub-page-stays-lit, 3-vs-4 tabs on extension, ordering, label localization, haptics, and the re-tap no-op.

One tiny thing, not blocking: the segment === 'settings' branch in activeTabFromPath is effectively dead today since sub-pages render in FullScreenPage — but the comment already calls that out as intentional defensive handling, so no change needed. LGTM.

@BrianSeong99

Copy link
Copy Markdown
Collaborator Author

Thanks — and you're right that it's worth acting on, though the branch is slightly more alive than it looks, so let me be precise about what changed.

segment === 'settings' was not dead: it's what lights the tab on /settings, which is the only Settings route that mounts TabLayout. What was speculative is the sub-page half — matching the segment meant the condition also covered /settings/<slug>, and those render in FullScreenPage, so they can never reach activeTabFromPath at all.

So rather than keep a condition whose extra reach is unreachable, I made it exact (b8895d8f):

// Exact, unlike the segment matches above: `/history/:programId` renders
// inside this shell, so Activity has sub-paths to stay lit for, whereas the
// only Settings route that mounts TabLayout is the bare root — every
// `/settings/<slug>` sub-page renders in FullScreenPage (see PageRouter).
if (pathname === '/settings') return 'settings';

The inconsistency with the browser / history segment matches above is deliberate and now documented: /history/:programId genuinely renders in the shell, so Activity has real sub-paths to stay lit for. Settings doesn't.

Two tests went with it, because both asserted a state the app cannot produce:

  • "keeps the settings tab active on a settings sub-page" now asserts the honest outcome (home) and says why.
  • "returns to the Settings root when tapping Settings from a sub-page" is gone — the to !== pathname branch it exercised is already covered by the Explore case.

150 tests pass across TabLayout / PageRouter / Settings. If sub-pages ever move into the tab shell, that's a PageRouter change and this line should change with it — which I'd rather have as a deliberate edit than as pre-installed handling nobody remembers is there.

@0xnullifier 0xnullifier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

Add Settings as a fourth bottom-navigation tab

3 participants