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
thisconnect
requested changes
May 18, 2026
|
|
||
| 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 = ...
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 🙏 |
thisconnect
requested changes
May 20, 2026
| onChangeTab={handleChangeTab} | ||
| /> | ||
| )} | ||
| <Outlet /> |
Collaborator
There was a problem hiding this comment.
This still uses Outlet from react-router, would it be possible to just use react children and use this as a container wrap the other components?
Collaborator
Author
There was a problem hiding this comment.
I tried replacing Outlet with children, but it remounts MarketplaceLayout between market/Bitsurance routes and brings back the tab blink.
| const isMarketSelect = pathname.startsWith('/market/select'); | ||
|
|
||
| return ( | ||
| <MarketProvider initialMarketAccountCode={isMarketSelect ? code : undefined}> |
Collaborator
There was a problem hiding this comment.
I think this could just always pass code={code} regardless of pathname.startsWith('/market/select');
then you could remove initialMarketAccountCode and don't need marketAccountCode
and don't need to do both validRouteAccountCode and validMarketAccountCode, but can just use code or am I missing something?
Collaborator
Author
There was a problem hiding this comment.
I tested this, and removing marketAccountCode loses the previously selected market account when switching from Buy/Sell to Bitsurance and back.
| const validRouteAccountCode = accounts.some(account => account.code === code) ? code : ''; | ||
| const validMarketAccountCode = accounts.some(account => account.code === marketAccountCode) ? marketAccountCode : ''; | ||
|
|
||
| const [selectedAccount, setSelectedAccount] = useState<string>(validRouteAccountCode || validMarketAccountCode || ''); |
Collaborator
There was a problem hiding this comment.
It looks to me that it would be simpler to just use "code" from the props and with this not need a state.
so just navigate instead of changing state with setSelectedAccount. But I made antoher comment later down about same concern.
| export const MarketplaceNavigation = ({ | ||
| accounts, | ||
| activeTab, | ||
| className = '', |
| return ( | ||
| <MarketTab | ||
| activeTab={activeTab} | ||
| className={`${style.navigation || ''} ${className}`} |
Collaborator
There was a problem hiding this comment.
If MarketTab should always have style.navigation remove className prop and always add it inside the MarketTab component directly.
| return tab; | ||
| } | ||
| return 'buy'; | ||
| }; |
Collaborator
There was a problem hiding this comment.
As this is not a webapp with user facing urls, but the uri's are controlled by the app itself, I find this function not so useful.
Also it confuses me that insure is not one of the tabs.
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.