frontend: move insurance into marketplace#4113
Open
strmci wants to merge 2 commits into
Open
Conversation
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request moves Bitsurance screens under /market/bitsurance and serves them via a new MarketplaceLayout with outlet context. Market navigation helpers and an "Insure" tab are added. Bitsurance pages are simplified from guided wrappers to View-based layouts and their internal routes updated to /market/bitsurance/*. Bitsurance entries were removed from the sidebar and settings; bottom-nav/back-navigation rules, tests, localization (generic.insure), and the changelog were updated accordingly. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontends/web/src/routes/bitsurance/bitsurance.tsx (1)
44-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways clear
scanLoadingon lookup failures.If
bitsuranceLookup()fails here, the function returns before resettingscanLoading, which leaves the detect CTA permanently disabled for the rest of the session.🔧 Suggested fix
const detect = async (redirectToDashboard: boolean) => { setScanLoading(true); setScanDone(false); setInsuredAccounts([]); - const response = await bitsuranceLookup(); - if (!response.success) { - alertUser(response.errorMessage); - return; - } - const insuredAccountsCodes = response.bitsuranceAccounts.map(account => account.status ? account.code : null); - const insured = accounts.filter(({ code }) => insuredAccountsCodes.includes(code)); - setInsuredAccounts(insured); - setScanDone(true); - setScanLoading(false); - if (insured.length && redirectToDashboard) { - navigate('/market/bitsurance/dashboard'); + try { + const response = await bitsuranceLookup(); + if (!response.success) { + alertUser(response.errorMessage); + return; + } + const insuredAccountsCodes = response.bitsuranceAccounts.map(account => account.status ? account.code : null); + const insured = accounts.filter(({ code }) => insuredAccountsCodes.includes(code)); + setInsuredAccounts(insured); + setScanDone(true); + if (insured.length && redirectToDashboard) { + navigate('/market/bitsurance/dashboard'); + } + } finally { + setScanLoading(false); } };🤖 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 `@frontends/web/src/routes/bitsurance/bitsurance.tsx` around lines 44 - 57, The detect function currently returns early on bitsuranceLookup() failure without resetting scanLoading, leaving the CTA disabled; update detect to ensure setScanLoading(false) is always called on error paths (including after alertUser(response.errorMessage) and any other early returns), e.g., call setScanLoading(false) before returning or use a try/finally around the bitsuranceLookup() flow so setScanLoading(false) runs regardless; refer to the detect function, bitsuranceLookup, setScanLoading and setScanDone to apply the fix.frontends/web/src/components/sidebar/sidebar.tsx (1)
95-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle account-scoped market routes in the sidebar active check.
This predicate only matches
/market..., so the Marketplace entry stays inactive on preserved account-scoped paths like/account/:code/market/....🔧 Suggested fix
- const userInSpecificAccountMarketPage = pathname.startsWith('/market'); + const userInSpecificAccountMarketPage = + pathname.startsWith('/market') + || /^\/account\/[^/]+\/market(?:\/|$)/.test(pathname);🤖 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 `@frontends/web/src/components/sidebar/sidebar.tsx` around lines 95 - 153, The sidebar currently sets userInSpecificAccountMarketPage = pathname.startsWith('/market'), which misses account-scoped market routes like /account/:code/market; update the predicate used by userInSpecificAccountMarketPage (and the NavLink market active check) to detect both top-level and account-scoped market paths — e.g., replace the startsWith check with a broader match (pathname.startsWith('/market') || pathname.includes('/market') or a regex like /(^\/market|\/account\/[^/]+\/market)/) and ensure the NavLink className for the market entry uses this updated variable to mark the link active.
🤖 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 `@frontends/web/src/routes/bitsurance/dashboard.tsx`:
- Around line 117-123: The Button's title prop is using the wrong translation
key (t('account.exportTransactions')) causing an unrelated tooltip; update the
title to use the bitsurance-related translation (e.g., set
title={t('bitsurance.dashboard.button')} or another appropriate bitsurance key)
on the Button component (the JSX around Button with className={style.button},
primary, onClick={() => navigate('/market/bitsurance/account')} and children
{t('bitsurance.dashboard.button')}) so the hover text matches the "Insure a new
account" CTA.
In `@frontends/web/src/routes/bitsurance/widget.tsx`:
- Around line 145-156: Replace the hardcoded iframe title "Bitsurance" with a
translated string: import and call useTranslation() in the widget component
(frontends/web/src/routes/bitsurance/widget.tsx), then replace
title="Bitsurance" with title={t('bitsurance.widgetTitle')} (or your chosen
translation key) and add the key to your i18n resource files; ensure iframeRef,
onIframeLoad, height and iframeURL usage remain unchanged.
In `@frontends/web/src/routes/router.tsx`:
- Around line 334-346: The nested Route with path="bitsurance" under the parent
Route path="bitsurance" creates a dead /bitsurance/bitsurance route; update the
child route inside the outer Route (the one rendering BitsuranceRedirect) to
either be removed or changed to a catch-all by replacing the child Route
path="bitsurance" with path="*" so it will match any legacy /bitsurance/...
paths and render BitsuranceRedirect; locate the Routes referencing
path="bitsurance" and the BitsuranceRedirect element to apply the fix.
---
Outside diff comments:
In `@frontends/web/src/components/sidebar/sidebar.tsx`:
- Around line 95-153: The sidebar currently sets userInSpecificAccountMarketPage
= pathname.startsWith('/market'), which misses account-scoped market routes like
/account/:code/market; update the predicate used by
userInSpecificAccountMarketPage (and the NavLink market active check) to detect
both top-level and account-scoped market paths — e.g., replace the startsWith
check with a broader match (pathname.startsWith('/market') ||
pathname.includes('/market') or a regex like
/(^\/market|\/account\/[^/]+\/market)/) and ensure the NavLink className for the
market entry uses this updated variable to mark the link active.
In `@frontends/web/src/routes/bitsurance/bitsurance.tsx`:
- Around line 44-57: The detect function currently returns early on
bitsuranceLookup() failure without resetting scanLoading, leaving the CTA
disabled; update detect to ensure setScanLoading(false) is always called on
error paths (including after alertUser(response.errorMessage) and any other
early returns), e.g., call setScanLoading(false) before returning or use a
try/finally around the bitsuranceLookup() flow so setScanLoading(false) runs
regardless; refer to the detect function, bitsuranceLookup, setScanLoading and
setScanDone to apply the fix.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: c20af321-9a3b-40c2-8e63-18820a32918b
📒 Files selected for processing (20)
frontends/web/src/app.tsxfrontends/web/src/components/bottom-navigation/bottom-navigation.tsxfrontends/web/src/components/bottom-navigation/utils.tsfrontends/web/src/components/sidebar/sidebar.tsxfrontends/web/src/contexts/BackNavigationContext.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/components/insuredtag.tsxfrontends/web/src/routes/bitsurance/account.tsxfrontends/web/src/routes/bitsurance/bitsurance.tsxfrontends/web/src/routes/bitsurance/dashboard.tsxfrontends/web/src/routes/bitsurance/widget.module.cssfrontends/web/src/routes/bitsurance/widget.tsxfrontends/web/src/routes/market/components/marketplace-navigation.tsxfrontends/web/src/routes/market/components/markettab.module.cssfrontends/web/src/routes/market/components/markettab.test.tsxfrontends/web/src/routes/market/components/markettab.tsxfrontends/web/src/routes/market/market.tsxfrontends/web/src/routes/market/marketplace-layout.tsxfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/more.tsx
💤 Files with no reviewable changes (2)
- frontends/web/src/app.tsx
- frontends/web/src/routes/bitsurance/widget.module.css
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
0a4f4b8 to
03f2bdb
Compare
strmci
force-pushed
the
move_insurance_to_marketplace
branch
5 times, most recently
from
a318c9e to
42e71f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontends/web/src/routes/router.tsx (1)
291-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd legacy
/bitsuranceroute redirects or update PR description.The PR objective states "redirect legacy
/bitsuranceroutes accordingly", but the current router contains no redirects for legacy paths like/bitsurance,/bitsurance/dashboard,/bitsurance/account/:code, or/bitsurance/widget/:code. These paths will now fail to match any route, breaking external links and user bookmarks.Either:
- Add a redirect subtree at the root
/bitsurancepath that maps these legacy routes to their new/market/bitsurance/*counterparts, or- Update the PR description to reflect that legacy URLs are intentionally no longer supported.
🤖 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 `@frontends/web/src/routes/router.tsx` around lines 291 - 319, The router is missing legacy redirects for /bitsurance/* so external links break; add a root-level Route subtree that matches legacy paths (e.g., path="bitsurance" with nested routes for index, dashboard, account/:code, widget/:code) and redirect them to their new counterparts under /market/bitsurance (use the same target routes used in the Market subtree such as the index -> /market/bitsurance, dashboard -> /market/bitsurance/dashboard, account/:code -> /market/bitsurance/account/:code, widget/:code -> /market/bitsurance/widget/:code) so existing bookmarks resolve to the new components (the new Market bitsurance routes reference Bitsurance, BitsuranceAccountEl, BitsuranceWidgetEl, BitsuranceDashboard and MarketplaceLayoutEl); alternatively update the PR description to state legacy URLs are intentionally removed.
🤖 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 `@frontends/web/src/components/sidebar/sidebar.tsx`:
- Line 95: The variable userInSpecificAccountMarketPage no longer matches its
predicate pathname.startsWith('/market'); rename it (e.g., inMarketSection) or
inline the condition where used to improve clarity—update all occurrences of
userInSpecificAccountMarketPage (including the second usage block within the
same file) to the new name or replace with pathname.startsWith('/market') so
references like the assignment and any conditionals or JSX checks remain
consistent.
In `@frontends/web/src/routes/bitsurance/account.tsx`:
- Around line 90-114: The JSX return wraps a single <View> in an unnecessary
fragment (<>...</>); remove the fragment tokens so the component returns the
<View> directly (keep the existing <ViewContent>, conditional rendering of
btcAccounts, GroupedAccountSelector props like disabled, selected,
onChange={handleChangeAccount}, onProceed={handleProceed}, and the
t('bitsuranceAccount.noAccount') branch unchanged). Ensure
parentheses/whitespace remain correct around the returned <View>.
In `@frontends/web/src/routes/bitsurance/bitsurance.tsx`:
- Around line 86-143: The top-level fragment wrapping the single <View> in the
Bitsurance component is unnecessary; remove the <> and </> so the render returns
the <View fullscreen={false}> element directly (update the return in
bitsurance.tsx to return <View>... without the surrounding fragment), keeping
all inner children and props unchanged.
In `@frontends/web/src/routes/bitsurance/dashboard.tsx`:
- Around line 105-197: The JSX fragment wrapping the single <View> is
unnecessary; remove the outer <>...</> so the component returns the <View>
directly (update the return in the render of this file: remove the fragment
tokens surrounding the View element), ensuring the remaining structure and
import usage of View, ViewContent and children stay intact.
In `@frontends/web/src/routes/market/marketplace-layout.tsx`:
- Around line 78-82: The Header currently always uses t('generic.buySell'),
causing Bitsurance pages to show the wrong title; update the title prop in
marketplace-layout's JSX (the Header component usage) to conditionally choose
t('generic.insure') when the page is a Bitsurance route (use the existing
pathname check, isBitsurance flag, or activeTab) and fall back to
t('generic.buySell') otherwise, ensuring the translation hook
useTranslation()/t() is used and keeping the existing HideAmountsButton
rendering unchanged.
---
Outside diff comments:
In `@frontends/web/src/routes/router.tsx`:
- Around line 291-319: The router is missing legacy redirects for /bitsurance/*
so external links break; add a root-level Route subtree that matches legacy
paths (e.g., path="bitsurance" with nested routes for index, dashboard,
account/:code, widget/:code) and redirect them to their new counterparts under
/market/bitsurance (use the same target routes used in the Market subtree such
as the index -> /market/bitsurance, dashboard -> /market/bitsurance/dashboard,
account/:code -> /market/bitsurance/account/:code, widget/:code ->
/market/bitsurance/widget/:code) so existing bookmarks resolve to the new
components (the new Market bitsurance routes reference Bitsurance,
BitsuranceAccountEl, BitsuranceWidgetEl, BitsuranceDashboard and
MarketplaceLayoutEl); alternatively update the PR description to state legacy
URLs are intentionally removed.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5e9759fe-e298-4b56-b496-1909e709ab2c
📒 Files selected for processing (21)
CHANGELOG.mdfrontends/web/src/app.tsxfrontends/web/src/components/bottom-navigation/bottom-navigation.tsxfrontends/web/src/components/bottom-navigation/utils.tsfrontends/web/src/components/sidebar/sidebar.tsxfrontends/web/src/contexts/BackNavigationContext.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/components/insuredtag.tsxfrontends/web/src/routes/bitsurance/account.tsxfrontends/web/src/routes/bitsurance/bitsurance.tsxfrontends/web/src/routes/bitsurance/dashboard.tsxfrontends/web/src/routes/bitsurance/widget.module.cssfrontends/web/src/routes/bitsurance/widget.tsxfrontends/web/src/routes/market/components/marketplace-navigation.tsxfrontends/web/src/routes/market/components/markettab.module.cssfrontends/web/src/routes/market/components/markettab.test.tsxfrontends/web/src/routes/market/components/markettab.tsxfrontends/web/src/routes/market/market.tsxfrontends/web/src/routes/market/marketplace-layout.tsxfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/more.tsx
💤 Files with no reviewable changes (2)
- frontends/web/src/app.tsx
- frontends/web/src/routes/bitsurance/widget.module.css
|
|
||
| const accountsByKeystore = getAccountsByKeystore(accounts); | ||
| const userInSpecificAccountMarketPage = (pathname.startsWith('/market')); | ||
| const userInSpecificAccountMarketPage = pathname.startsWith('/market'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: rename userInSpecificAccountMarketPage.
Now that the predicate is just pathname.startsWith('/market'), the variable's name (which implied a per-account market context) no longer reflects what it checks. Consider renaming to inMarketSection or inlining the condition.
♻️ Proposed rename
- const userInSpecificAccountMarketPage = pathname.startsWith('/market');
+ const inMarketSection = pathname.startsWith('/market');
@@
- className={({ isActive }) => isActive || userInSpecificAccountMarketPage ? style.sidebarActive : ''}
+ className={({ isActive }) => isActive || inMarketSection ? style.sidebarActive : ''}Also applies to: 151-169
🤖 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 `@frontends/web/src/components/sidebar/sidebar.tsx` at line 95, The variable
userInSpecificAccountMarketPage no longer matches its predicate
pathname.startsWith('/market'); rename it (e.g., inMarketSection) or inline the
condition where used to improve clarity—update all occurrences of
userInSpecificAccountMarketPage (including the second usage block within the
same file) to the new name or replace with pathname.startsWith('/market') so
references like the assignment and any conditionals or JSX checks remain
consistent.
Collaborator
There was a problem hiding this comment.
would be great to use simple naming, to reduce mental overhead, inMarket or inMarketSection 👍
const inMarket = ...There was a problem hiding this comment.
@thisconnect, agreed — inMarket keeps it concise. Here's the updated diff:
- const userInSpecificAccountMarketPage = pathname.startsWith('/market');
+ const inMarket = pathname.startsWith('/market');
@@
- className={({ isActive }) => isActive || userInSpecificAccountMarketPage ? style.sidebarActive : ''}
+ className={({ isActive }) => isActive || inMarket ? style.sidebarActive : ''}🧠 Learnings used
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4102
File: frontends/web/src/components/infobutton/infobutton.tsx:6-6
Timestamp: 2026-05-05T13:22:38.824Z
Learning: In this repository’s web frontend, accessibility (screen reader support and meaningful ARIA semantics such as aria-labels/roles) is intentionally deferred until an active accessibility effort starts. During code reviews, do not raise or require missing ARIA attributes, roles, or screen-reader support as issues in TSX changes; wait to enforce these concerns once accessibility work is underway.
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 3916
File: frontends/web/src/components/banners/offline-error.tsx:16-17
Timestamp: 2026-05-14T09:39:14.878Z
Learning: In BitBoxSwiss/bitbox-wallet-app, treat `TConfig.backend` and `TConfig.backend.proxy` as required (non-optional) because they are typed as such in `frontends/web/src/api/config.ts`. Only the top-level `config` value from `ConfigContext` may be `undefined` (`TConfig | undefined`). During review, avoid recommending or introducing optional chaining like `config?.backend?.` or `config?.backend?.proxy`—if you need to handle absence, guard on `config` itself (or earlier null-check) and then access `backend`/`proxy` directly.
Collaborator
Author
There was a problem hiding this comment.
renamed to inMarketSection
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
42e71f8 to
9160192
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@frontends/web/src/routes/bitsurance/bitsurance.tsx`:
- Around line 48-52: The bitsuranceLookup result is not being used to stop or
redirect before navigation: update the logic around bitsuranceLookup(),
alertUser(), and detect(false) so that you only call
navigate('/market/bitsurance/account') when the lookup succeeded and the account
actually needs onboarding; if response.success is false, return early (no
navigation) after calling alertUser(response.errorMessage); if the response
indicates the account is already insured, either short-circuit to the
appropriate account view or show the "already insured" UI instead of proceeding
to the insure flow. Adjust both the initial lookup block (bitsuranceLookup,
alertUser, detect(false)) and the later navigation block that currently
unconditionally calls navigate('/market/bitsurance/account') so navigation is
gated by response.success and the lookup's insured status.
In `@frontends/web/src/routes/market/components/markettab.module.css`:
- Around line 1-9: The .navigation rule uses justify-content: center (a no-op
without a flex/grid container) and suggests it was intended to be both a flex
item and a flex container (flex-grow: 0); update the .navigation CSS to make
justify-content effective by adding display: flex (and optionally align-items:
center if vertical centering of the pill buttons is desired) so children are
centered, or remove justify-content if you do not want it to be a container —
edit the .navigation block to add display: flex and align-items: center to
restore the intended centering behavior.
In `@frontends/web/src/routes/market/components/markettab.test.tsx`:
- Line 76: Tests are failing because the i18n mock used by frontends/web tests
does not include translations for 'generic.swap' and 'generic.insure', so t(...)
falls back to the key strings and the queries for button names 'Swap'/'Insure'
fail; open the i18n mock (frontends/web/__mocks__/i18n.ts) and add entries
'generic.swap': 'Swap' and 'generic.insure': 'Insure' to the resources object
used by the mocked t function so the rendered buttons have the expected
accessible names.
In `@frontends/web/src/routes/market/market.tsx`:
- Around line 75-90: The effect currently selects nextAccount but never updates
the URL when the route lacked a valid account code; update it to perform a
replace-navigation to the account-scoped path whenever validRouteAccountCode is
falsy and you choose a non-empty nextAccount: after calling
setMarketAccountCode?.(nextAccount) and setSelectedAccount(nextAccount) in the
useEffect, call your router replace (e.g. history.replace or navigate with {
replace: true }) with getMarketSelectPath(activeTab, nextAccount) so the URL is
normalized into the account-scoped path; guard this navigation behind
validRouteAccountCode being empty and nextAccount !== '' to avoid unnecessary
redirects.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 024b5c9a-1805-4ee9-b76c-816761377a95
📒 Files selected for processing (21)
CHANGELOG.mdfrontends/web/src/app.tsxfrontends/web/src/components/bottom-navigation/bottom-navigation.tsxfrontends/web/src/components/bottom-navigation/utils.tsfrontends/web/src/components/sidebar/sidebar.tsxfrontends/web/src/contexts/BackNavigationContext.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/components/insuredtag.tsxfrontends/web/src/routes/bitsurance/account.tsxfrontends/web/src/routes/bitsurance/bitsurance.tsxfrontends/web/src/routes/bitsurance/dashboard.tsxfrontends/web/src/routes/bitsurance/widget.module.cssfrontends/web/src/routes/bitsurance/widget.tsxfrontends/web/src/routes/market/components/marketplace-navigation.tsxfrontends/web/src/routes/market/components/markettab.module.cssfrontends/web/src/routes/market/components/markettab.test.tsxfrontends/web/src/routes/market/components/markettab.tsxfrontends/web/src/routes/market/market.tsxfrontends/web/src/routes/market/marketplace-layout.tsxfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/more.tsx
💤 Files with no reviewable changes (2)
- frontends/web/src/routes/bitsurance/widget.module.css
- frontends/web/src/app.tsx
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
9160192 to
2ef541b
Compare
Route Bitsurance through the marketplace layout and add the Insure tab.Move Bitsurance entry points to /market/bitsurance, update navigationentry points, and keep the single-account auto-connect flow. Also preserve account-code marketplace tab paths, clean stale no-account routing, and cover the new tab/path helpers in tests.
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
2ef541b to
7b7ab45
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontends/web/src/routes/market/components/markettab.module.css (1)
1-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
display: flexto makejustify-content: centereffective.The
.navigationrule includesjustify-content: center, but withoutdisplay: flex, this property has no effect. Theflex-grow: 0on line 3 confirms this element is intended to participate in both flex-item and flex-container roles.🛠️ Proposed fix
.navigation { box-sizing: border-box; + display: flex; flex-grow: 0; justify-content: center; margin: 0 auto; max-width: var(--content-width); padding: 0 var(--space-default); width: 100%; }🤖 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 `@frontends/web/src/routes/market/components/markettab.module.css` around lines 1 - 9, The .navigation CSS block is missing display: flex so justify-content: center has no effect; update the .navigation rule to include display: flex (keeping existing properties like flex-grow: 0, width, padding, etc.) so it becomes a flex container and the justify-content center behavior works as intended.
🤖 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 `@frontends/web/src/routes/market/components/marketplace-navigation.tsx`:
- Line 20: cachedShowSwap is a module-level boolean preserving swap visibility
across remounts; replace it with a proper shared state provider to be safe for
SSR/concurrent renders. Create a SwapVisibilityContext (or use existing global
store like Redux/Zustand) that exposes showSwap and setShowSwap, update
MarketplaceNavigation to read/write via useContext/useStore instead of the
module variable, remove cachedShowSwap, and optionally persist initial value to
localStorage inside the provider if you need cross-session caching.
In `@frontends/web/src/routes/market/marketplace-layout.tsx`:
- Around line 41-72: The pathname checks using startsWith in
marketplace-layout.tsx (variables isBitsurance, isMarketSelect, and
showMarketplaceNavigation) are too permissive; update them to anchor with exact
match or a trailing slash (e.g., replace
pathname.startsWith('/market/bitsurance') with pathname === '/market/bitsurance'
|| pathname.startsWith('/market/bitsurance/')) and do the same for
'/market/select' and the '/market/bitsurance/widget' check so sibling segments
like '/market/bitsurance-foo' no longer match; keep the rest of the logic
(marketAccountCode, outletContext, handleChangeTab) unchanged.
---
Duplicate comments:
In `@frontends/web/src/routes/market/components/markettab.module.css`:
- Around line 1-9: The .navigation CSS block is missing display: flex so
justify-content: center has no effect; update the .navigation rule to include
display: flex (keeping existing properties like flex-grow: 0, width, padding,
etc.) so it becomes a flex container and the justify-content center behavior
works as intended.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 23d82b71-52c4-4f7f-8657-38a0029d0392
📒 Files selected for processing (22)
CHANGELOG.mdfrontends/web/__mocks__/i18n.tsfrontends/web/src/app.tsxfrontends/web/src/components/bottom-navigation/bottom-navigation.tsxfrontends/web/src/components/bottom-navigation/utils.tsfrontends/web/src/components/sidebar/sidebar.tsxfrontends/web/src/contexts/BackNavigationContext.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/components/insuredtag.tsxfrontends/web/src/routes/bitsurance/account.tsxfrontends/web/src/routes/bitsurance/bitsurance.tsxfrontends/web/src/routes/bitsurance/dashboard.tsxfrontends/web/src/routes/bitsurance/widget.module.cssfrontends/web/src/routes/bitsurance/widget.tsxfrontends/web/src/routes/market/components/marketplace-navigation.tsxfrontends/web/src/routes/market/components/markettab.module.cssfrontends/web/src/routes/market/components/markettab.test.tsxfrontends/web/src/routes/market/components/markettab.tsxfrontends/web/src/routes/market/market.tsxfrontends/web/src/routes/market/marketplace-layout.tsxfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/more.tsx
💤 Files with no reviewable changes (2)
- frontends/web/src/app.tsx
- frontends/web/src/routes/bitsurance/widget.module.css
| showSwap?: boolean; | ||
| }; | ||
|
|
||
| let cachedShowSwap: boolean | undefined; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Module-level state is acceptable here but consider alternatives in future.
The cachedShowSwap module-level variable preserves swap visibility across component remounts. While this works for a client-side SPA, consider moving this to a context provider or state manager if the app adopts SSR or concurrent rendering in the future.
🤖 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 `@frontends/web/src/routes/market/components/marketplace-navigation.tsx` at
line 20, cachedShowSwap is a module-level boolean preserving swap visibility
across remounts; replace it with a proper shared state provider to be safe for
SSR/concurrent renders. Create a SwapVisibilityContext (or use existing global
store like Redux/Zustand) that exposes showSwap and setShowSwap, update
MarketplaceNavigation to read/write via useContext/useStore instead of the
module variable, remove cachedShowSwap, and optionally persist initial value to
localStorage inside the provider if you need cross-session caching.
Collaborator
There was a problem hiding this comment.
This introduced a impure hidden global state, this should be move to the market context, but see my other comment about the current approach of using react-router "outlet".
There was a problem hiding this comment.
@thisconnect, agreed — cachedShowSwap as a module-level variable is global and impure state that survives across component lifecycles (and even multiple app instances in a test environment, which can cause flaky tests). Moving it into the marketplace/market context is the right fix, where it can live as proper reactive state alongside marketAccountCode, regions, etc. in TMarketplaceOutletContext.
Since this refactor seems blocked on or coupled to the broader outlet-vs-context discussion you mentioned in your other comment, would you like me to open a follow-up GitHub issue to track moving cachedShowSwap into the market context once the outlet approach is settled?
🧠 Learnings used
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4038
File: frontends/web/src/routes/settings/components/about/support-link-setting.tsx:7-7
Timestamp: 2026-04-22T18:36:13.978Z
Learning: In files under `frontends/web/src/routes/`, do not flag relative imports within the `src/routes/` directory tree (e.g., `./...` and `../...`). Use the `@/` alias mainly when the import leaves `src/routes/` and targets other `src/` areas (e.g., `@/components`, `@/api`, `@/utils`).
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4102
File: frontends/web/src/components/infobutton/infobutton.tsx:6-6
Timestamp: 2026-05-05T13:22:38.824Z
Learning: In this repository’s web frontend, accessibility (screen reader support and meaningful ARIA semantics such as aria-labels/roles) is intentionally deferred until an active accessibility effort starts. During code reviews, do not raise or require missing ARIA attributes, roles, or screen-reader support as issues in TSX changes; wait to enforce these concerns once accessibility work is underway.
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 3916
File: frontends/web/src/components/banners/offline-error.tsx:16-17
Timestamp: 2026-05-14T09:39:14.878Z
Learning: In BitBoxSwiss/bitbox-wallet-app, treat `TConfig.backend` and `TConfig.backend.proxy` as required (non-optional) because they are typed as such in `frontends/web/src/api/config.ts`. Only the top-level `config` value from `ConfigContext` may be `undefined` (`TConfig | undefined`). During review, avoid recommending or introducing optional chaining like `config?.backend?.` or `config?.backend?.proxy`—if you need to handle absence, guard on `config` itself (or earlier null-check) and then access `backend`/`proxy` directly.
Collaborator
There was a problem hiding this comment.
We should fix it and not merge as is, so no issue is needed thanks.
There was a problem hiding this comment.
🧩 Analysis chain
Understood, @thisconnect — let me look at the full content of both files to plan a minimal fix.
🏁 Script executed:
#!/bin/bash
cat frontends/web/src/routes/market/marketplace-layout.tsx
echo "---FILE BOUNDARY---"
cat frontends/web/src/routes/market/components/marketplace-navigation.tsx
Length of output: 5447
@thisconnect, agreed. Here's the minimal fix — lift the swap-status fetch and its cached value into MarketplaceLayout state, so MarketplaceNavigation becomes a pure/stateless forwarder.
- Move
showSwapstate andgetSwapStatusload toMarketplaceLayout - Remove
cachedShowSwapmodule-level var +useLoad/useEffectfromMarketplaceNavigation - Wire the resolved value down as a prop
🧠 Learnings used
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4038
File: frontends/web/src/routes/settings/components/about/support-link-setting.tsx:7-7
Timestamp: 2026-04-22T18:36:13.978Z
Learning: In files under `frontends/web/src/routes/`, do not flag relative imports within the `src/routes/` directory tree (e.g., `./...` and `../...`). Use the `@/` alias mainly when the import leaves `src/routes/` and targets other `src/` areas (e.g., `@/components`, `@/api`, `@/utils`).
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4102
File: frontends/web/src/components/infobutton/infobutton.tsx:6-6
Timestamp: 2026-05-05T13:22:38.824Z
Learning: In this repository’s web frontend, accessibility (screen reader support and meaningful ARIA semantics such as aria-labels/roles) is intentionally deferred until an active accessibility effort starts. During code reviews, do not raise or require missing ARIA attributes, roles, or screen-reader support as issues in TSX changes; wait to enforce these concerns once accessibility work is underway.
Learnt from: thisconnect
Repo: BitBoxSwiss/bitbox-wallet-app PR: 3916
File: frontends/web/src/components/banners/offline-error.tsx:16-17
Timestamp: 2026-05-14T09:39:14.878Z
Learning: In BitBoxSwiss/bitbox-wallet-app, treat `TConfig.backend` and `TConfig.backend.proxy` as required (non-optional) because they are typed as such in `frontends/web/src/api/config.ts`. Only the top-level `config` value from `ConfigContext` may be `undefined` (`TConfig | undefined`). During review, avoid recommending or introducing optional chaining like `config?.backend?.` or `config?.backend?.proxy`—if you need to handle absence, guard on `config` itself (or earlier null-check) and then access `backend`/`proxy` directly.
- 📌 Commit the changes to this branch
Collaborator
Author
There was a problem hiding this comment.
React Router outlet context usage replaced with a market-specific React context, following the staging-spark style.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
thisconnect
requested changes
May 18, 2026
| en: { | ||
| translation: { | ||
| 'generic.insure': 'Insure', | ||
| 'generic.swap': 'Swap', |
Collaborator
There was a problem hiding this comment.
Is it important to add this here?
Other tests don't mock here, but directly in the test files where needed.
| <div className={style.single}> | ||
| <ShieldLight alt={t('sidebar.insurance')} /> | ||
| </div> | ||
| <span className={style.sidebarLabel}>{t('sidebar.insurance')}</span> |
Collaborator
There was a problem hiding this comment.
With this PR sidebar.insurance is not used anywhere anymore, please remove.
| <Main> | ||
| <GuideWrapper> | ||
| <GuidedContent> | ||
| <Header title={<h2>{t('bitsuranceAccount.title')}</h2>} /> |
Collaborator
There was a problem hiding this comment.
bitsuranceAccount.title is not used in the codebase anymore.
| const { isDarkMode } = useDarkmode(); | ||
| const [insuredAccounts, setInsuredAccounts] = useState<TAccount[]>([]); | ||
| const [redirecting, setRedirecting] = useState(true); | ||
| const [redirecting, setRedirecting] = useState(() => accounts.some(({ bitsuranceStatus }) => bitsuranceStatus)); |
Collaborator
There was a problem hiding this comment.
I wanted to nit that this initialization is a bit hard to read and duplicate with the condition in the next effect.
But GPT mentions you might not need the redirecting state at all.
But should it only check the accounts on component-mount or also if accounts object changes later?
Collaborator
Author
There was a problem hiding this comment.
Good point, I removed the redirecting state and replaced it with a derived hasBitsuranceAccount value, which is used both for the redirect effect and the render guard.
| if (nextAccount) { | ||
| setMarketAccountCode?.(nextAccount); | ||
| if (!validRouteAccountCode) { | ||
| navigate(getMarketSelectPath(activeTab, nextAccount), { replace: true }); |
Collaborator
There was a problem hiding this comment.
I'd find it much clearer to understand what it does if you just use the 1 line template literal and remove getMarketSelectPath.
Collaborator
Author
There was a problem hiding this comment.
Removed getMarketSelectPath and used market select URL template strings.
| showSwap?: boolean; | ||
| }; | ||
|
|
||
| let cachedShowSwap: boolean | undefined; |
Collaborator
There was a problem hiding this comment.
This introduced a impure hidden global state, this should be move to the market context, but see my other comment about the current approach of using react-router "outlet".
|
|
||
| const handleChangeTab = (tab: TMarketplaceTab) => { | ||
| if (tab === 'insure') { | ||
| navigate(getInsurancePath(accounts)); |
Collaborator
There was a problem hiding this comment.
getInsurancePath is only used here, I find it much harder to follow. Please remove getInsurancePath and just add the 2 lines here.
Collaborator
Author
There was a problem hiding this comment.
getInsurancePath removed
|
|
||
| const accountsByKeystore = getAccountsByKeystore(accounts); | ||
| const userInSpecificAccountMarketPage = (pathname.startsWith('/market')); | ||
| const userInSpecificAccountMarketPage = pathname.startsWith('/market'); |
Collaborator
There was a problem hiding this comment.
would be great to use simple naming, to reduce mental overhead, inMarket or inMarketSection 👍
const inMarket = ...| setSelectedAccount(nextAccount); | ||
| } | ||
| }, [accounts, selectedAccount]); | ||
| }, [accounts, activeTab, navigate, selectedAccount, setMarketAccountCode, validRouteAccountCode]); |
Collaborator
There was a problem hiding this comment.
The purpose of this effect before was to: // keep account list in sync and ensure a valid selected account..
Now it also does conditional navigate but is very hard to follow. I can't tell if there was also some bug fixed or why it needs all those dependecies here.
Collaborator
Author
There was a problem hiding this comment.
I split the effect so each part has a single responsibility (syncing the account list, ensuring a valid selected account, and normalizing the URL) so it's easier to follow.
| </View> | ||
| </> | ||
| ); | ||
| }; |
Collaborator
There was a problem hiding this comment.
I've reviewed all other changes, but did not yet fully understand this file nor review it fully.
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
d326e55 to
0b1d8c4
Compare
- remove generic.insure / generic.swap from the shared i18n mock and kept translation setup local to the relevant test. - rename userInSpecificAccountMarketPage to inMarketSection. - remove redirecting state in Bitsurance and derived redirect state from hasBitsuranceAccount. - removed getMarketSelectPath and used explicit market select URL template strings. - remove getInsurancePath and inlined the Bitsurance route decision. - removed unused sidebar.insurance, bitsuranceAccount.title - replaced hidden global cachedShowSwap with market- scoped React context. - replaced React Router outlet context usage with a market-specific React context, following the staging-spark style. - simplify market navigation state in market.tsx
strmci
force-pushed
the
move_insurance_to_marketplace
branch
from
0b1d8c4 to
3bfc0bb
Compare
Collaborator
Author
|
@thisconnect thanks I addressed your comments in the second commit, PTAL 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Route Bitsurance through the marketplace layout and add the Insure tab.Move Bitsurance entry points to
/market/bitsurance, update navigationentry points, and keep the single-account auto-connect flow.
Also preserve account-code marketplace tab paths, clean stale no-account routing, and cover the new tab/path
helpers in tests.