Skip to content

fix navigation between tabs - #162

Merged
bordalix merged 1 commit into
masterfrom
fix_tab_navigation_2
Oct 15, 2025
Merged

bordalix merged 1 commit into
masterfrom
fix_tab_navigation_2

Conversation

@bordalix

@bordalix bordalix commented Oct 15, 2025 •

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where the Wallet, Apps, and Settings tabs could remain hidden after switching. These tabs now reliably become visible when selected, ensuring consistent navigation and proper UI synchronization.
    • Improves overall tab behavior and reduces cases where users needed to reload or switch away-and-back to see content. Other tabs are unaffected.

@coderabbitai

coderabbitai Bot commented Oct 15, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Synchronizes tab UI by restoring DOM classes for three active tabs in src/App.tsx. In the tab-syncing useEffect, when Wallet, Apps, or Settings becomes active, the code removes the 'tab-hidden' class from the corresponding tab element. No exported APIs or broader control flow changed.

Changes

Cohort / File(s) Summary
Tab DOM class restoration
src/App.tsx
In useEffect syncing current tab, added removal of 'tab-hidden' for Wallet, Apps, and Settings tab elements when they become active; no other logic altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • fix tab navigation #159 — Also changes 'tab-hidden' handling in src/App.tsx for Wallet/Apps/Settings, complementing this PR by adjusting the opposite state when tabs are inactive.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “fix navigation between tabs” clearly reflects the main change of restoring tab visibility classes to ensure proper tab switching behavior, aligning directly with the added DOM class restoration logic in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_tab_navigation_2

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying wallet-signet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 84c0ea9
Status: ✅  Deploy successful!
Preview URL: https://8fe13196.wallet-23u.pages.dev
Branch Preview URL: https://fix-tab-navigation-2.wallet-23u.pages.dev

View logs

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying wallet-mutinynet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 84c0ea9
Status: ✅  Deploy successful!
Preview URL: https://ce1b0923.arkade-wallet.pages.dev
Branch Preview URL: https://fix-tab-navigation-2.arkade-wallet.pages.dev

View logs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/App.tsx (1)

80-103: LGTM! Fix correctly ensures active tab visibility.

The additions of classList.remove('tab-hidden') for each active tab properly synchronize the DOM state with the tab navigation. This ensures the active tab is always visible after switching, fixing the navigation issue.

The implementation is correct, though the pattern repeats three times. If you'd like to reduce duplication, consider this optional refactor:

  useEffect(() => {
+   const tabs = {
+     [Tabs.Wallet]: walletRef,
+     [Tabs.Apps]: appsRef,
+     [Tabs.Settings]: settingsRef,
+   }
+   
+   const activeTab = tabs[tab]
+   if (activeTab?.current) {
+     activeTab.current.setActive()
+     activeTab.current.classList.remove('tab-hidden')
+     
+     Object.entries(tabs).forEach(([key, ref]) => {
+       if (key !== tab && ref.current) {
+         ref.current.classList.add('tab-hidden')
+       }
+     })
+   }
-   switch (tab) {
-     case Tabs.Wallet:
-       walletRef.current?.setActive()
-       walletRef.current?.classList.remove('tab-hidden')
-       appsRef.current?.classList.add('tab-hidden')
-       settingsRef.current?.classList.add('tab-hidden')
-       break
-     case Tabs.Apps:
-       appsRef.current?.setActive()
-       appsRef.current?.classList.remove('tab-hidden')
-       walletRef.current?.classList.add('tab-hidden')
-       settingsRef.current?.classList.add('tab-hidden')
-       break
-     case Tabs.Settings:
-       settingsRef.current?.setActive()
-       settingsRef.current?.classList.remove('tab-hidden')
-       walletRef.current?.classList.add('tab-hidden')
-       appsRef.current?.classList.add('tab-hidden')
-       break
-     default:
-       break
-   }
  }, [tab])
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c66340a and 84c0ea9.

📒 Files selected for processing (1)
  • src/App.tsx (1 hunks)

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying wallet-bitcoin-v8 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 84c0ea9
Status: ✅  Deploy successful!
Preview URL: https://bd4b174f.wallet-v8.pages.dev
Branch Preview URL: https://fix-tab-navigation-2.wallet-v8.pages.dev

View logs

@bordalix
bordalix merged commit a1a8b6d into master Oct 15, 2025
1 check passed
@bordalix
bordalix deleted the fix_tab_navigation_2 branch October 15, 2025 13:51
@coderabbitai coderabbitai Bot mentioned this pull request Dec 22, 2025
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.

1 participant