Repository navigation
Conversation
… is legacy The legacy group only renders while "Show legacy markets" is on, and that flag reset to false on every mount. A user viewing a legacy market therefore could not see it in the switcher and nothing appeared selected. Default the flag from the market being viewed instead. Closes aave#3128
|
@Krshivam25 is attempting to deploy a commit to the Aave Team on Vercel. A member of the Team first needs to authorize it. |
| const toggleFavoriteMarket = useRootStore((store) => store.toggleFavoriteMarket); | ||
| const favoriteMarkets = useRootStore((store) => store.favoriteMarkets); | ||
|
|
||
| // A legacy market the user is actually on must not be missing from the list: the legacy |
There was a problem hiding this comment.
Accepted — good catch, and the case is stronger than described.
setQueryParameter uses Router.push(..., { shallow: true }), so a market change never remounts the header. On top of that, src/store/root.ts hydrates currentMarket from ?marketName/localStorage inside document.onreadystatechange, i.e. after the store initialises to availableMarkets[0] (Core, non-legacy). If the switcher mounts before that fires, the initialiser latches false on exactly the deep-link path #3128 is about. My manual check only passed because hydration won that race locally.
Applied your suggestion. The effect only ever sets true and only on a currentMarket change, so a deliberate toggle-off is still respected while the user stays on that market; the lazy initialiser is kept so there's no flash when the market is already known at mount.
One behaviour worth flagging: because the effect never sets false, once a legacy market has been visited the section stays expanded for the rest of that mount, even after switching to a non-legacy market (a fresh page load still starts collapsed). I verified all three paths locally. Mirroring both ways would override a user's deliberate "on" toggle whenever they moved to a non-legacy market, which seemed worse — happy to change it if you'd prefer the stricter behaviour.
The lazy initialiser runs only at mount, but the header survives shallow market changes and the store hydrates marketName from the URL after the first render, so the default could latch a stale false and hide the active legacy market again. Sync it whenever the current market becomes legacy.
| const toggleFavoriteMarket = useRootStore((store) => store.toggleFavoriteMarket); | ||
| const favoriteMarkets = useRootStore((store) => store.favoriteMarkets); | ||
|
|
||
| // A legacy market the user is actually on must not be missing from the list: the legacy |
There was a problem hiding this comment.
Accepted — good catch, and the case is stronger than described.
setQueryParameter uses Router.push(..., { shallow: true }), so a market change never remounts the header. On top of that, src/store/root.ts hydrates currentMarket from ?marketName/localStorage inside document.onreadystatechange, i.e. after the store initialises to availableMarkets[0] (Core, non-legacy). If the switcher mounts before that fires, the initialiser latches false on exactly the deep-link path #3128 is about. My manual check only passed because hydration won that race locally.
Applied your suggestion. The effect only ever sets true and only on a currentMarket change, so a deliberate toggle-off is still respected while the user stays on that market; the lazy initialiser is kept so there's no flash when the market is already known at mount.
One behaviour worth flagging: because the effect never sets false, once a legacy market has been visited the section stays expanded for the rest of that mount, even after switching to a non-legacy market (a fresh page load still starts collapsed). I verified all three paths locally. Mirroring both ways would override a user's deliberate "on" toggle whenever they moved to a non-legacy market, which seemed worse — happy to change it if you'd prefer the stricter behaviour.
The legacy group only renders while "Show legacy markets" is on, and that flag reset to false on every mount. A user viewing a legacy market therefore could not see it in the switcher and nothing appeared selected. Default the flag from the market being viewed instead.
Closes #3128
General Changes
Developer Notes
Root cause —
showLegacywas initialised tofalseand never persisted, so it reset on every mount regardless of the market being viewed. The legacy group only renders while that flag is on, andisSelectedcan only highlight an item that is rendered.Change — two parts:
useRootStorereads (at its previous positioncurrentMarketis not yet defined, so referencing it there is a temporal-dead-zone error) and derives its default from the current market, reusing the existing module-levelgetMarketCategoryhelper.The effect is not redundant with the initialiser. setQueryParameter uses Router.push(..., { shallow: true }), so a market change never remounts the header; and src/store/root.ts hydrates currentMarket from ?marketName/localStorage inside document.onreadystatechange, i.e. after the store initialises to availableMarkets[0] (Core, non-legacy). Without the effect, a switcher that mounts before hydration latches a stale false on exactly the deep-link path this issue describes. The initialiser is kept so there is no flash when the market is already known at mount.
This came from the automated review on the first commit; the finding was accepted and 077a5e6 addresses it.
Behaviour worth flagging — the effect only ever sets true, so once a legacy market has been visited the section stays expanded for the rest of that mount, even after switching to a non-legacy market. A fresh page load on a non-legacy market still starts collapsed. Mirroring both ways would override a user's deliberate "on" toggle whenever they moved to a non-legacy market, which seemed worse — happy to switch to the stricter behaviour if you prefer it.
tsc --noEmit and next lint are clean for this file (exhaustivjest baseline is unchanged.
Relationship to #3129 — independent. That PR fixes the search half of the same area (#3127) and touches the import block, noResults and the legacyrender guard; this one touches the React import and the state declaration. They merge in either order. No unit test here for the same reason: the pure helpers introduced by #3129 live in a file that does not exisng it here would make the two PRs conflict. Happy to follow up with a test once #3129 lands.
Pre-existing failures — the 4 jest suites in src/utils/ that fail on main (a wagmi/chains import issue) are unaffected.
Reviewer Checklist
Please ensure you, as the reviewer(s), have gone through this checklist to ensure that the code changes are ready to ship safely and to help mitigate
any downstream issues that may occur.