Repository navigation
refactor(ui): show pill navbar only on root pages - #474
Conversation
When VITE_DEV_NSEC is set in .env.local, automatically initializes the wallet on dev server startup, bypassing onboarding and unlock screens.
The pill navbar was a fixed overlay requiring manual padding on every
component with bottom-positioned elements. Since all action buttons
live on sub-pages (Send, Receive, Settings options), hiding the navbar
on sub-pages eliminates the conflict entirely.
- Show navbar only on Wallet home, Apps home, and Settings menu
- Hide with spring animation (Framer Motion) on sub-page navigation
- Remove footer clearance CSS hacks and manual padding workarounds
in ButtonsOnBottom and Delegates
- Disable pointer-events on hidden navbar to prevent click blocking
Files: App.tsx, PillNavbarOverlay.tsx, ButtonsOnBottom.tsx,
Delegates.tsx, index.css, ionic.css
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughApp now computes navbar visibility from current screen and Settings option, passes a new Changes
Sequence DiagramsequenceDiagram
participant App as App.tsx
participant Overlay as PillNavbarOverlay
participant Motion as Framer Motion
App->>App: compute isSettingsRoot & showNavbar (screen + option)
App->>Overlay: render with visible = showNavbar
alt visible = true
Overlay->>Motion: animate to { opacity: 1, y: 0 }
Motion-->>Overlay: mount visible layer
Overlay->>Overlay: ensure interactive (no inert / pointer-events active)
else visible = false
Overlay->>Motion: animate to { opacity: 0, y: 24 }
Motion-->>Overlay: collapsed/hidden
Overlay->>Overlay: apply inert and pill-navbar-layer--hidden (disable pointer-events)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
9d9119d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3e4dffe3.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://wt-navbar-root-only-20260325.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
9d9119d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd8c7722.arkade-wallet.pages.dev |
| Branch Preview URL: | https://wt-navbar-root-only-20260325.arkade-wallet.pages.dev |
- Keep master's authState/unlockWallet additions in App.tsx and wallet.tsx - Keep master's .scroll-fade opt-in selector for scroll mask - Remove footer clearance rule (our change) that master re-added
🔍 Arkana PR Review —
|
The navbar visibility check used `screen` from NavigationContext, which lags behind the actual rendered page. When the app is locked, `screen` stays as Pages.Wallet while `page` is overridden to Pages.Unlock, causing the navbar to appear on the unlock screen. Add `page === screen` guard so the navbar hides whenever the rendered page is overridden (unlock, loading). Also add `inert` to the hidden navbar overlay for accessibility, and stub VITE_DEV_NSEC in tests to prevent env-dependent flakiness.
|
Iterative review — Good improvement. The key change in const isSettingsRoot = screen === Pages.Settings && option === SettingsOptions.Menu
const showNavbar = page === screen && (screen === Pages.Wallet || screen === Pages.Apps || isSettingsRoot)The New tests cover all three cases (unlock, loading, authenticated root) — solid coverage. Minor: the LGTM ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/App.test.tsx (1)
193-235: LGTM!The new test suite provides solid coverage of the navbar visibility logic:
- Tests the
page !== screenguard (unlock overrides wallet screen).- Tests the loading hold scenario.
- Tests the happy path for authenticated users on root pages.
Consider adding tests for Apps and Settings root pages if not covered elsewhere, to ensure parity with the Wallet root test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/App.test.tsx` around lines 193 - 235, Add two new tests mirroring the Wallet root test to cover Apps and Settings roots: use renderApp with authState: 'authenticated' and initialized: true and set screen/tab to Pages.Apps/Tabs.Apps and Pages.Settings/Tabs.Settings respectively; then await screen.findByTestId('ion-app') and assert ionApp.className contains 'has-pill-navbar'. Place them alongside the existing Navbar visibility tests and reuse the same test setup (beforeEach stubbing detectJSCapabilities, matchMedia, vi.stubEnv) so renderApp, Pages, and Tabs symbols are referenced exactly as in the current suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/App.test.tsx`:
- Around line 193-235: Add two new tests mirroring the Wallet root test to cover
Apps and Settings roots: use renderApp with authState: 'authenticated' and
initialized: true and set screen/tab to Pages.Apps/Tabs.Apps and
Pages.Settings/Tabs.Settings respectively; then await
screen.findByTestId('ion-app') and assert ionApp.className contains
'has-pill-navbar'. Place them alongside the existing Navbar visibility tests and
reuse the same test setup (beforeEach stubbing detectJSCapabilities, matchMedia,
vi.stubEnv) so renderApp, Pages, and Tabs symbols are referenced exactly as in
the current suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8c4b040-9af2-4fe7-a4b1-f929222d99a2
📒 Files selected for processing (5)
src/App.tsxsrc/components/PillNavbarOverlay.tsxsrc/index.csssrc/ionic.csssrc/test/App.test.tsx
💤 Files with no reviewable changes (1)
- src/ionic.css
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/PillNavbarOverlay.tsx
With the navbar hidden on sub-pages, payment success screens (send, receive, notes) were dead ends with no way to navigate home. Adds a ButtonsOnBottom "Sounds good" button that navigates to wallet.
|
Iterative review — Straightforward fix for the dead-end problem: Send, Receive, and Notes success screens now have a Consistent across all three screens, correct imports. No concerns — this was the expected follow-up from hiding the navbar on sub-pages. LGTM ✅ |
Addresses CodeRabbit nitpick — adds navbar visibility tests for Apps root and Settings menu to match the existing Wallet root test.
🔍 Arkana PR ReviewPR: refactor(ui): show pill navbar only on root pages Overall AssessmentWell-structured refactor that eliminates a fragile layout pattern (manual padding workarounds for the fixed navbar). The core logic change — hiding navbar on sub-pages using Observations1. 2. 3. "Sounds good" button on success screens ✅ 4. 5. Spring animation 6. Removed padding workarounds ✅ 7. Tests ✅ 8. Minor: SecurityNo security concerns — UI layout changes only. VerdictLooks good. No blocking issues. The |
🔍 Iterative Review — commits
|
|
@sahilc0 fix merge conflicts |
|
Review of new commit (
Follows the same pattern established by the earlier commits for send/receive/notes success screens. Consistent and necessary — without it, the faucet success screen was a dead end after the navbar-hiding change. LGTM. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/screens/mocks.ts (1)
117-120: Mock is incomplete but functional due toas anycast.The
mockOptionsContextValueonly provides 2 of the 6 fields required byOptionsContextProps(direction,options,goBack, andvalidOptionsare missing persrc/providers/options.tsx:130-146). This works because consumers cast withas any, but it could cause runtime errors if tests ever access the missing fields.Consider completing the mock for robustness:
♻️ Suggested complete mock
export const mockOptionsContextValue = { + direction: 'forward' as const, option: SettingsOptions.Menu, + options: [], + goBack: () => {}, setOption: () => {}, + validOptions: () => [], }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/screens/mocks.ts` around lines 117 - 120, The mock mockOptionsContextValue is missing several fields from OptionsContextProps (direction, options, goBack, validOptions) which are currently masked by an as any cast; update mockOptionsContextValue to include all required keys with sensible defaults (e.g., direction: a default string, options: an array, goBack: a noop, validOptions: a function returning true) so tests don't rely on any, and remove any as any casts in tests/consumers so the mock type-checks against OptionsContextProps; reference the OptionsContextProps type and the mockOptionsContextValue constant when making the change.src/test/App.test.tsx (2)
219-260: Good coverage but missing negative test for Settings sub-pages.The tests cover the positive cases well. However, there's no test verifying that the navbar is hidden on Settings sub-pages (when
option !== SettingsOptions.Menu). Per the logic insrc/App.tsx:251,isSettingsRootrequiresoption === SettingsOptions.Menu.🧪 Suggested test for Settings sub-page
it('shows navbar on settings menu when authenticated and initialized', async () => { // ... existing test }) + + it('hides navbar on settings sub-page when authenticated and initialized', async () => { + renderApp({ + authState: 'authenticated', + initialized: true, + screen: Pages.Settings, + tab: Tabs.Settings, + option: SettingsOptions.Password, // or any non-Menu option + }) + + const ionApp = await screen.findByTestId('ion-app') + expect(ionApp.className).not.toContain('has-pill-navbar') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/App.test.tsx` around lines 219 - 260, Add a negative test that verifies the navbar is hidden for Settings sub-pages by rendering via renderApp with authState: 'authenticated', initialized: true, screen: Pages.Settings, tab: Tabs.Settings and option set to a non-root value (e.g., SettingsOptions.Profile or any value !== SettingsOptions.Menu) and then assert that the ion-app test id's className does NOT contain 'has-pill-navbar'; this mirrors the existing positive Settings root test but uses SettingsOptions != Menu to exercise the isSettingsRoot branch.
198-217: Consider extracting shared setup to reduce duplication.The
beforeEach/afterEachblocks here duplicate the setup from the "App startup routing" suite (lines 116-135). This could be extracted to a shared helper or module-level setup.♻️ Suggested refactor to deduplicate setup
+function setupTestEnvironment() { + sessionStorage.clear() + window.matchMedia = vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn(), + })) + vi.mocked(detectJSCapabilities).mockResolvedValue({ isSupported: true }) + vi.stubEnv('VITE_DEV_NSEC', '') +} + describe('App startup routing', () => { beforeEach(() => { - sessionStorage.clear() - window.matchMedia = vi.fn().mockImplementation((query: string) => ({ - matches: false, - media: query, - onchange: null, - addListener: vi.fn(), - removeListener: vi.fn(), - addEventListener: vi.fn(), - removeEventListener: vi.fn(), - dispatchEvent: vi.fn(), - })) - vi.mocked(detectJSCapabilities).mockResolvedValue({ isSupported: true }) - vi.stubEnv('VITE_DEV_NSEC', '') + setupTestEnvironment() }) // ... }) describe('Navbar visibility', () => { beforeEach(() => { - sessionStorage.clear() - window.matchMedia = vi.fn().mockImplementation((query: string) => ({ - matches: false, - media: query, - onchange: null, - addListener: vi.fn(), - removeListener: vi.fn(), - addEventListener: vi.fn(), - removeEventListener: vi.fn(), - dispatchEvent: vi.fn(), - })) - vi.mocked(detectJSCapabilities).mockResolvedValue({ isSupported: true }) - vi.stubEnv('VITE_DEV_NSEC', '') + setupTestEnvironment() }) // ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/App.test.tsx` around lines 198 - 217, The beforeEach/afterEach setup in the 'Navbar visibility' suite duplicates initialization from the 'App startup routing' suite; extract the shared setup into a single helper or module-level beforeEach to avoid duplication: create a shared setup function (e.g., initTestEnv or setupAppTest) that performs sessionStorage.clear(), stubs window.matchMedia, mocks detectJSCapabilities, and sets vi.stubEnv('VITE_DEV_NSEC', ''), then call that helper from both suites and move vi.unstubAllEnvs() into a shared afterEach or module-level teardown to keep cleanup consistent; update references to detectJSCapabilities, window.matchMedia, sessionStorage.clear, vi.stubEnv and vi.unstubAllEnvs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/App.test.tsx`:
- Around line 219-260: Add a negative test that verifies the navbar is hidden
for Settings sub-pages by rendering via renderApp with authState:
'authenticated', initialized: true, screen: Pages.Settings, tab: Tabs.Settings
and option set to a non-root value (e.g., SettingsOptions.Profile or any value
!== SettingsOptions.Menu) and then assert that the ion-app test id's className
does NOT contain 'has-pill-navbar'; this mirrors the existing positive Settings
root test but uses SettingsOptions != Menu to exercise the isSettingsRoot
branch.
- Around line 198-217: The beforeEach/afterEach setup in the 'Navbar visibility'
suite duplicates initialization from the 'App startup routing' suite; extract
the shared setup into a single helper or module-level beforeEach to avoid
duplication: create a shared setup function (e.g., initTestEnv or setupAppTest)
that performs sessionStorage.clear(), stubs window.matchMedia, mocks
detectJSCapabilities, and sets vi.stubEnv('VITE_DEV_NSEC', ''), then call that
helper from both suites and move vi.unstubAllEnvs() into a shared afterEach or
module-level teardown to keep cleanup consistent; update references to
detectJSCapabilities, window.matchMedia, sessionStorage.clear, vi.stubEnv and
vi.unstubAllEnvs accordingly.
In `@src/test/screens/mocks.ts`:
- Around line 117-120: The mock mockOptionsContextValue is missing several
fields from OptionsContextProps (direction, options, goBack, validOptions) which
are currently masked by an as any cast; update mockOptionsContextValue to
include all required keys with sensible defaults (e.g., direction: a default
string, options: an array, goBack: a noop, validOptions: a function returning
true) so tests don't rely on any, and remove any as any casts in tests/consumers
so the mock type-checks against OptionsContextProps; reference the
OptionsContextProps type and the mockOptionsContextValue constant when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d50166b7-5131-4ff3-b11a-c87af961f77a
📒 Files selected for processing (3)
src/screens/Wallet/Receive/Amount.tsxsrc/test/App.test.tsxsrc/test/screens/mocks.ts
Resolve merge conflicts after merging master into navbar branch: - App.tsx: keep `option` from OptionsContext (navbar visibility) + `dataReady`/`loadError` from master (boot animation) - App.tsx: integrate boot animation logic with navbar show/hide - Tests: fix loading assertions (LoadingLogo no longer renders visible gitCommit text), add dataReady to mock, extract shared test setup, add negative test for settings sub-pages, complete mockOptionsContextValue with all required fields
|
Merge conflicts resolved and pushed. Also addressed CodeRabbit feedback:
@bordalix merge conflicts fixed ^ |
|
Review — merge commit Clean merge. The navbar visibility logic is properly preserved alongside master's new boot animation ( Key observations on the conflict resolution:
No issues found. The merge preserves both feature branches correctly. |
|
Followup review — Clean adaptation of e2e tests to the new navbar behavior:
No issues. These are mechanical test adjustments that correctly follow the UI changes from the parent commits. |
🔍 Automated Review Update (2026-04-02)New commits since last review: test fixes by @bordalix + merge with master. Core navbar refactor logic unchanged. Changes reviewed (165f5c4 → 9d9119d)
Previous review findings still applyThe original review covered:
Test coverageTests updated appropriately — navbar visibility tests cover Wallet root, Apps root, Settings menu (shown), Settings sub-page (hidden), unlock screen (hidden), and loading hold (hidden). E2E tests updated to use "Sounds good" navigation and explicit back-button clicks where navbar is now hidden. No security concerns. LGTM — ready for human approval. 👍 |
Summary
page === screenguard ensures the navbar is hidden whenever the rendered page is overridden (unlock, loading hold)inerton hidden navbar — when the navbar is hidden, it's removed from the accessibility tree and tab order4rempadding inButtonsOnBottom, manualpill-navbar-clearancepadding inDelegates.tsxVITE_DEV_NSECauto-init feature (separate commit) for faster local dev iterationWhy
The pill navbar was a fixed overlay (
position: fixed, portaled todocument.body) that required every component with bottom-positioned elements to manually account for its height. This was brittle — three CSS variables, automatic padding onIonContent, margin onIonFooter, conditional padding inButtonsOnBottom, and manual padding inDelegates.tsx.Since all action buttons exist on sub-pages and never on root pages, hiding the navbar on sub-pages eliminates the conflict entirely.
The navbar was also appearing on the Unlock screen because the visibility check used
screenfrom NavigationContext (which could still bePages.Wallet) instead of the actual renderedpage. Addingpage === screenas a guard fixes this.Success screens (send, receive, notes) became dead ends after hiding the navbar on sub-pages — no navbar, no back button, no way out. Added a "Sounds good"
ButtonsOnBottombutton on each that navigates back to wallet home, following the same pattern asInit/Success.tsx.Note for reviewers
The
isSettingsRootcheck inApp.tsxcouples navbar visibility toOptionsContext.option. This means if someone refactors Settings sub-navigation, they need to be aware it affects navbar visibility. Worth considering whether this should live in the navigation system instead (e.g., exposeisRootPagefromNavigationContext). @pietro909 thoughts?Test plan
Summary by CodeRabbit
Style
New Features
Tests