Repository navigation
fix navigation - #869
fix navigation#869
Conversation
WalkthroughThe change removes ChangesSettings navigation consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Navigation cleanup: WalletSettings removal
Pure UI/navigation change — no VTXO handling, signing, forfeit, round lifecycle, or unilateral exit code is touched. No protocol-critical concerns.
Correctness
The core simplification is sound. WalletSettings was a second Pages enum entry pointing to the same <Settings/> component, used as a workaround to let the gear button push Settings as a "forward" page while Settings itself remained a root/tab in ROOT_PAGES. Removing WalletSettings and also removing Pages.Settings from ROOT_PAGES unifies both paths cleanly.
Trace through the new flow is correct:
- Home gear →
navigate(Pages.Settings)→ forward push (backStack:[Pages.Wallet]) - Browser back from Settings menu → popstate → backStack pop →
Pages.Wallet✓ - Settings back button →
navigate(Pages.Wallet)→ root navigation, clears backStack ✓ subNavHandlerdelegation inhandlePopStatestill checksPages.Settings✓
replace() calls in Detail.tsx, MintSuccess.tsx, and Assets/Settings.tsx correctly simplify from [Pages.Settings, Pages.WalletSettings] to [Pages.Settings] since only one entry can ever exist in the backStack now.
No remaining WalletSettings references in the codebase — grep confirms the diff covers all nine sites.
Issues
App.test.tsx:214-224 — duplicate test after the rename
The test named "hides navbar on wallet-launched settings when authenticated and initialized" is changed from Pages.WalletSettings to Pages.Settings. After this PR, it has identical inputs to the test immediately above it at lines 202-212 ("hides navbar on settings menu when authenticated and initialized"). Both pass screen: Pages.Settings, option: SettingsOptions.Menu. One of these should be removed, or the surviving test name updated to remove the now-meaningless "wallet-launched" distinction.
Settings/Menu.tsx:22 — hardcoded back target
<Header text='Settings' backFunc={() => navigate(Pages.Wallet)} />navigate(Pages.Wallet) is a root navigation that clears the entire backStack. This works correctly while Settings is only reachable from HomeHeader. If any future path navigates to Pages.Settings from a non-Wallet screen (e.g., a post-init flow), the back button will skip over the intermediate page and go straight to Wallet. Not a current bug, but worth a comment noting the assumption, or using goBack() for resilience.
Pages enum ordinal shift (navigation.tsx)
Removing WalletSettings (previously between Vtxos/Wallet and WalletSwap by alpha order) renumbers WalletSwap. Since these values are only held in-memory (not serialized to storage or sent over the wire), this is safe.
No cross-repo impact
The Pages enum is internal to the wallet app; grepping the SDK clones (ts-sdk, go-sdk, rust-sdk, dotnet-sdk) finds no consumers.
Two nits (duplicate test, hardcoded back comment), but neither blocks the fix. The navigation logic is correct.
Deploying wallet-mutinynet with
|
| Latest commit: |
de2d64d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c304cba9.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-navigation.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
de2d64d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff3af193.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-navigation.wallet-bitcoin.pages.dev |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 41b5867 — 1 commit since f0cfcea; 1 prior issue fixed, 1 still open.
Prior issue closeout
[1] Duplicate test — Fixed (commit 41b5867 "remove duplicated test")
The test "hides navbar on wallet-launched settings when authenticated and initialized" (src/test/App.test.tsx:214-224 in f0cfcea) has been removed. It had screen: Pages.Settings, option: SettingsOptions.Menu — identical inputs to the surviving test at lines 202-212 ("hides navbar on settings menu when authenticated and initialized"). Correct removal; the remaining test provides full coverage of this case.
[2] Hardcoded back target — Still open (src/screens/Settings/Menu.tsx:22, unchanged)
<Header text='Settings' backFunc={() => navigate(Pages.Wallet)} />Not touched by the new commit. Still a nit: if Settings is ever reachable from a non-Wallet screen the back button will skip over the intermediate page. Not a current bug given the single entry-point, but the assumption is silent. No change since last pass.
Incremental diff assessment
The only change in this push is the deletion of the duplicate test. No new code was introduced. No protocol logic, signing, VTXO handling, or navigation logic was modified. No new issues.
Nothing blocking in this pass. The one remaining nit (hardcoded back target) is pre-existing and non-blocking.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/navigation.tsx`:
- Line 88: Update the navigation history handling around ROOT_PAGES and
handlePopState to distinguish browser Back from Forward, using history.state or
a forward stack so Wallet → Settings → Back → Forward restores Settings when
backStack is empty. Add a regression test covering this complete navigation
sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c50a3443-b409-490f-b928-3cf454350fc1
📒 Files selected for processing (8)
src/providers/navigation.tsxsrc/screens/Apps/Assets/Detail.tsxsrc/screens/Apps/Assets/MintSuccess.tsxsrc/screens/Apps/Assets/Settings.tsxsrc/screens/Settings/Index.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Wallet/HomeHeader.tsxsrc/test/App.test.tsx
💤 Files with no reviewable changes (1)
- src/test/App.test.tsx
|
|
||
| // Root pages - switches between these get no animation | ||
| const ROOT_PAGES = new Set([Pages.Wallet, Pages.Settings]) | ||
| const ROOT_PAGES = new Set([Pages.Wallet]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add forward-history handling for the non-root Pages.Settings route.
When Pages.Settings is excluded from ROOT_PAGES, navigating from Wallet adds a browser history entry. handlePopState still treats every popstate as Back. After Back returns to Wallet, Forward fires with an empty backStack, so the screen remains Wallet.
Track navigation direction with history.state or a forward stack. Add a regression test for Wallet → Settings → browser Back → browser Forward.
Also applies to: 229-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/navigation.tsx` at line 88, Update the navigation history
handling around ROOT_PAGES and handlePopState to distinguish browser Back from
Forward, using history.state or a forward stack so Wallet → Settings → Back →
Forward restores Settings when backStack is empty. Add a regression test
covering this complete navigation sequence.
Source: Coding guidelines
For some reason clanker added a second page for settings (WalletSettings) and kept the old one (Settings) active.
Than he was trying to juggle between them, making a mess.
I removed WalletSettings and now navigation works as expected:
Navigation works with Back and Forward in the browser, as expected.
Replaces #866
@tiero please review
Summary by CodeRabbit