Repository navigation
deal with browser back button - #271
Conversation
WalkthroughReplaced direct navigate state updates with a browser-backed navigation stack (useRef). Added push/pop helpers, history.pushState usage, and a popstate listener to synchronize browser back/forward with in-app screen/tab state while avoiding forbidden pages (e.g., InitConnect/Loading). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-21T18:25:36.391ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/navigation.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/navigation.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (4)
src/providers/navigation.tsx (4)
1-1: LGTM!The additional imports (
useEffectanduseRef) are necessary for the browser history navigation feature.
181-184: Verify browser history behavior.The
historycheck on line 182 is redundant in browser environments (history is always defined). More importantly, every call tonavigatepushes a new browser history entry. After navigating through multiple pages and clicking back through all of them, one additional back click will exit the app entirely.Consider using
history.replaceStatefor the first navigation to prevent exiting the app after navigating backward through all pages:const push = (page: Pages) => { - if (history) history.pushState({}, '', '') + if (typeof window !== 'undefined' && window.history) { + window.history.pushState({}, '', '') + } navigationHistory.current.push(page) }Alternatively, to fully prevent exiting the app via back button, consider pushing an initial dummy state on mount and using
replaceStatefor the first navigation.
186-192: LGTM!The
popfunction correctly retrieves the previous page from the history stack and defaults toPages.Initwhen the stack is empty.
198-202: LGTM!The
navigatefunction correctly integrates with the new history mechanism by callingpushbefore updating the screen and tab state.
Deploying wallet-mutinynet with
|
| Latest commit: |
5122883
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54cf1fe5.arkade-wallet.pages.dev |
| Branch Preview URL: | https://back-button.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
5122883
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5614af78.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://back-button.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/providers/navigation.tsx (2)
181-184: Consider preventing duplicate consecutive history entries.The
pushfunction always adds the page to the history stack, even if it's already the current page. This creates duplicate entries (e.g.,[Init, A, A]) that affect back button behavior—users would need to click back multiple times to navigate away from the same page.Apply this diff to skip pushing duplicate consecutive entries:
const push = (page: Pages) => { + const lastPage = navigationHistory.current[navigationHistory.current.length - 1] + if (lastPage === page) return if (history) history.pushState({}, '', '') navigationHistory.current.push(page) }Also applies to: 202-206
182-182: Use more defensive check forhistoryobject.The
if (history)check works but could be more defensive and consistent with the window check on line 196.Apply this diff for better defensive coding:
- if (history) history.pushState({}, '', '') + if (typeof window !== 'undefined' && window.history) { + window.history.pushState({}, '', '') + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/navigation.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/navigation.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages: wallet-mutinynet
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/providers/navigation.tsx (1)
179-184: Consider consistent SSR guard pattern.Line 182 uses
if (history)while line 202 usestypeof window !== 'undefined'. Both work, but the latter is more defensive in SSR environments where accessing undeclared globals may throw in certain bundler configurations.const push = (page: Pages) => { - if (history) history.pushState({}, '', '') + if (typeof window !== 'undefined') window.history.pushState({}, '', '') navigationHistory.current.push(page) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/navigation.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/navigation.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (3)
src/providers/navigation.tsx (3)
1-1: LGTM!Import additions correctly support the new navigation history implementation.
186-198: LGTM!The
popfunction correctly addresses all previous review concerns:
- Wrapped in
useCallbackwith appropriate empty dependency array- Guards against emptying the history stack
- Prevents back navigation to
InitConnect- State updates are properly sequenced
200-212: LGTM!
- The
useEffectproperly cleans up thepopstatelistener, fixing the previous memory leak concern- The dependency array correctly includes
pop- The
navigatefunction properly sequences history push before state updates
Clicking back on the browser navigates inside the app.
@tiero please review
@theDavidCoen please test
Closes #255
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.