fix: category selection #532
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR removes the footer-outlet registration system and replaces it with a PageCta type and PageCtaButton; CTA objects are returned from hooks and contexts and rendered directly by pages. Earn-page selection/state and position-details initialization were refactored to align with the new CTA flow. ChangesFooter Outlet Removal and PageCta Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/widget/src/pages/review/hooks/use-pending-review.hook.ts (1)
151-158: ⚡ Quick winDrop the CTA
useMemounless a consumer needs identity stability.This object is derived from local state and translations only. Under the React Compiler guideline, prefer a plain
PageCtahere and keep memoization for boundaries that truly need stable identity.As per coding guidelines,
packages/widget/src/**/*.{ts,tsx}: "React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity."🤖 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 `@packages/widget/src/pages/review/hooks/use-pending-review.hook.ts` around lines 151 - 158, The CTA object is unnecessarily wrapped in useMemo; remove the useMemo wrapper and return a plain PageCta object named cta constructed from t("shared.confirm"), onClickRef.current, disabled: false, and isLoading: actionPendingMutation.isPending so its identity is not artificially memoized; update references to the existing cta variable (created where useMemo was) and remove useMemo import if now unused.Source: Coding guidelines
packages/widget/src/pages/complete/hooks/use-complete.hook.ts (1)
58-68: ⚡ Quick winDrop the CTA
useMemounless a consumer needs identity stability.This is just local render data in the supplied context. With React Compiler enabled, prefer a plain
PageCtaobject here and only memoize at the boundary that actually depends on referential stability.As per coding guidelines,
packages/widget/src/**/*.{ts,tsx}: "React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity."🤖 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 `@packages/widget/src/pages/complete/hooks/use-complete.hook.ts` around lines 58 - 68, The CTA is unnecessarily wrapped in useMemo (const cta = useMemo<PageCta>(...)) despite being local render state; remove the useMemo and return a plain PageCta object instead (preserve properties: disabled, isLoading, label: t("complete.continue", { context: isLedgerLive ? "ledger" : undefined }), onClick: () => onClickRef.current(), hide: !!activityReviewMatch) so consumers that do need referential stability can be memoized at their boundary; update any references to the cta variable to use the plain object and remove useMemo import if now unused.Source: Coding guidelines
packages/widget/src/pages-dashboard/position-details/components/position-details-actions.tsx (1)
85-114: ⚡ Quick winDrop the CTA
useMemounless a consumer needs identity stability.Here the value is immediately consumed as a render prop for
PageCtaButton, so a plainPageCtaobject is the better fit under the React Compiler rule.As per coding guidelines,
packages/widget/src/**/*.{ts,tsx}: "React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity."🤖 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 `@packages/widget/src/pages-dashboard/position-details/components/position-details-actions.tsx` around lines 85 - 114, Remove the unnecessary useMemo and replace unstakeCta with a plain PageCta value computed inline (still returning null when isLoading is true) since identity stability isn't required by consumers; keep the same fields (disabled: unstakeDisabled, isLoading: false, label: t(...getExtendedYieldType(integrationData)), onClick: onUnstakeClick) and the Maybe.fromRecord mapping logic or its equivalent so PageCtaButton receives identical props but without wrapping in useMemo.Source: Coding guidelines
packages/widget/src/pages/review/hooks/use-action-review.hook.ts (1)
146-154: ⚡ Quick winDrop the CTA
useMemounless a consumer needs identity stability.Nothing in the supplied context shows a boundary that requires this object to stay referentially stable. Prefer a plain
PageCtavalue and memoize only where identity is actually observed.As per coding guidelines,
packages/widget/src/**/*.{ts,tsx}: "React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity."🤖 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 `@packages/widget/src/pages/review/hooks/use-action-review.hook.ts` around lines 146 - 154, The CTA object currently created with useMemo (const cta = useMemo<PageCta>(...)) should be converted to a plain PageCta value since no consumer requires referential identity; remove the useMemo call and return a simple object for cta with the same properties (label: t(`activity.review.${labelKey}`), onClick: () => navigate(`/activity/${path}/steps`), disabled: false, isLoading: false, hide: actionOlderThan7Days), keeping the same referenced symbols (cta, PageCta, labelKey, navigate, path, actionOlderThan7Days, t) so behavior is unchanged while complying with the React Compiler guideline against unnecessary useMemo.Source: Coding guidelines
packages/widget/src/pages/steps/hooks/use-steps.hook.ts (1)
175-186: ⚡ Quick winDrop the CTA
useMemounless a consumer needs identity stability.The new CTA is plain derived render state in the supplied code. Prefer returning a normal
PageCtavalue here and reserve manual memoization for an actual identity-sensitive boundary.As per coding guidelines,
packages/widget/src/**/*.{ts,tsx}: "React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity."🤖 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 `@packages/widget/src/pages/steps/hooks/use-steps.hook.ts` around lines 175 - 186, The CTA currently wrapped in useMemo (const cta = useMemo<PageCta>(...)) should be converted to a plain derived value: remove the useMemo import/usage and return a direct PageCta (or null) based on txStates.length, preserving the same shape (disabled, isLoading, label: t("shared.cancel"), onClick calling onClickRef.current(), variant: "secondary") and keeping the PageCta type annotation; this drops unnecessary identity memoization while retaining onClickRef, txStates and t usage.Source: Coding guidelines
packages/widget/src/pages/review/hooks/use-stake-review.hook.ts (1)
198-212: ⚡ Quick winPrefer a plain CTA object here.
The hook already returns a fresh object on Line 231, so memoizing only
ctadoes not buy semantic stability and just adds dependency bookkeeping. As per coding guidelines,packages/widget/src/**/*.{ts,tsx}: React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity.🤖 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 `@packages/widget/src/pages/review/hooks/use-stake-review.hook.ts` around lines 198 - 212, The cta is unnecessarily wrapped in useMemo; remove the useMemo call and return a plain PageCta object instead — construct cta as { disabled: kycGateIsBlocking, isLoading: enterMutation.isPending || yieldKycGate.isLoading, label: t("shared.confirm"), onClick: () => onClickRef.current() } so you still reference onClickRef, enterMutation, yieldKycGate, kycGateIsBlocking and t but avoid memo/dependency bookkeeping; ensure the hook's returned value remains the fresh object used later.Source: Coding guidelines
packages/widget/src/pages/details/earn-page/state/earn-page-context.tsx (1)
751-789: ⚡ Quick winDrop the
useMemoaround this CTA.
EarnPageContext.Providerstill receives a freshvalueobject below, so memoizing onlyctadoes not provide the semantic stability the compiler guideline allows it for. A plainconst cta = ...is simpler here. As per coding guidelines,packages/widget/src/**/*.{ts,tsx}: React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity.🤖 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 `@packages/widget/src/pages/details/earn-page/state/earn-page-context.tsx` around lines 751 - 789, Remove the useMemo wrapper around the cta variable and replace it with a plain const cta = ... expression so the CTA is computed directly (keep the same conditional structure and all referenced symbols: buttonCTAText, buttonDisabled, isConnected, isLedgerLiveAccountPlaceholder, externalProviders, appLoading, isFetching, yieldKycGate.isLoading, onClickRef, connectClickRef, registerFooterButton, hasNotYieldsForToken, and t); this aligns with the React Compiler guideline because EarnPageContext.Provider still creates a fresh value object and there is no semantic identity requirement that needs memoization.Source: Coding guidelines
packages/widget/src/pages/review/hooks/use-unstake-review.hook.ts (1)
159-167: ⚡ Quick winPrefer a plain CTA object here.
This hook also returns a new object below, so memoizing just
ctadoes not create any semantic-stability guarantee for consumers. As per coding guidelines,packages/widget/src/**/*.{ts,tsx}: React Compiler is enabled. Do not adduseMemo,useCallback, orReact.memoonly for render-performance optimization; prefer plain values/functions. Use manual memoization only when required for semantic stability, such as an external API dependency or context value identity.🤖 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 `@packages/widget/src/pages/review/hooks/use-unstake-review.hook.ts` around lines 159 - 167, The cta currently uses useMemo (const cta = useMemo<PageCta>(...)) which is unnecessary and prevents semantic-stability guarantees because the hook already returns a new object later; replace the useMemo with a plain PageCta object (const cta: PageCta = { label: t("shared.confirm"), onClick: () => onClickRef.current(), disabled: kycGateIsBlocking, isLoading: unstakeIsLoading || yieldKycGate.isLoading }) so you return a regular value and preserve the same referenced symbols (cta, PageCta, onClickRef, kycGateIsBlocking, unstakeIsLoading, yieldKycGate.isLoading) without using useMemo/useCallback.Source: Coding guidelines
🤖 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 `@packages/widget/src/pages/details/earn-page/state/earn-page-context.tsx`:
- Around line 476-480: onTokenBalanceSelect currently always dispatches { type:
"token/select" } which ignores dashboard context and can change
selectedDashboardYieldCategory; instead, read selectedDashboardYieldCategory
from state (or props) and when a dashboard category is active dispatch the
category-aware action (e.g. { type: "dashboard/token/select", data: { token:
tokenBalance.token, category: selectedDashboardYieldCategory } } or the existing
category-aware action your reducer expects) otherwise fall back to dispatching {
type: "token/select", data: tokenBalance.token }; update the
onTokenBalanceSelect useCallback to branch on selectedDashboardYieldCategory and
call the appropriate dispatch so token picks on dashboard flows preserve the
active category.
---
Nitpick comments:
In
`@packages/widget/src/pages-dashboard/position-details/components/position-details-actions.tsx`:
- Around line 85-114: Remove the unnecessary useMemo and replace unstakeCta with
a plain PageCta value computed inline (still returning null when isLoading is
true) since identity stability isn't required by consumers; keep the same fields
(disabled: unstakeDisabled, isLoading: false, label:
t(...getExtendedYieldType(integrationData)), onClick: onUnstakeClick) and the
Maybe.fromRecord mapping logic or its equivalent so PageCtaButton receives
identical props but without wrapping in useMemo.
In `@packages/widget/src/pages/complete/hooks/use-complete.hook.ts`:
- Around line 58-68: The CTA is unnecessarily wrapped in useMemo (const cta =
useMemo<PageCta>(...)) despite being local render state; remove the useMemo and
return a plain PageCta object instead (preserve properties: disabled, isLoading,
label: t("complete.continue", { context: isLedgerLive ? "ledger" : undefined }),
onClick: () => onClickRef.current(), hide: !!activityReviewMatch) so consumers
that do need referential stability can be memoized at their boundary; update any
references to the cta variable to use the plain object and remove useMemo import
if now unused.
In `@packages/widget/src/pages/details/earn-page/state/earn-page-context.tsx`:
- Around line 751-789: Remove the useMemo wrapper around the cta variable and
replace it with a plain const cta = ... expression so the CTA is computed
directly (keep the same conditional structure and all referenced symbols:
buttonCTAText, buttonDisabled, isConnected, isLedgerLiveAccountPlaceholder,
externalProviders, appLoading, isFetching, yieldKycGate.isLoading, onClickRef,
connectClickRef, registerFooterButton, hasNotYieldsForToken, and t); this aligns
with the React Compiler guideline because EarnPageContext.Provider still creates
a fresh value object and there is no semantic identity requirement that needs
memoization.
In `@packages/widget/src/pages/review/hooks/use-action-review.hook.ts`:
- Around line 146-154: The CTA object currently created with useMemo (const cta
= useMemo<PageCta>(...)) should be converted to a plain PageCta value since no
consumer requires referential identity; remove the useMemo call and return a
simple object for cta with the same properties (label:
t(`activity.review.${labelKey}`), onClick: () =>
navigate(`/activity/${path}/steps`), disabled: false, isLoading: false, hide:
actionOlderThan7Days), keeping the same referenced symbols (cta, PageCta,
labelKey, navigate, path, actionOlderThan7Days, t) so behavior is unchanged
while complying with the React Compiler guideline against unnecessary useMemo.
In `@packages/widget/src/pages/review/hooks/use-pending-review.hook.ts`:
- Around line 151-158: The CTA object is unnecessarily wrapped in useMemo;
remove the useMemo wrapper and return a plain PageCta object named cta
constructed from t("shared.confirm"), onClickRef.current, disabled: false, and
isLoading: actionPendingMutation.isPending so its identity is not artificially
memoized; update references to the existing cta variable (created where useMemo
was) and remove useMemo import if now unused.
In `@packages/widget/src/pages/review/hooks/use-stake-review.hook.ts`:
- Around line 198-212: The cta is unnecessarily wrapped in useMemo; remove the
useMemo call and return a plain PageCta object instead — construct cta as {
disabled: kycGateIsBlocking, isLoading: enterMutation.isPending ||
yieldKycGate.isLoading, label: t("shared.confirm"), onClick: () =>
onClickRef.current() } so you still reference onClickRef, enterMutation,
yieldKycGate, kycGateIsBlocking and t but avoid memo/dependency bookkeeping;
ensure the hook's returned value remains the fresh object used later.
In `@packages/widget/src/pages/review/hooks/use-unstake-review.hook.ts`:
- Around line 159-167: The cta currently uses useMemo (const cta =
useMemo<PageCta>(...)) which is unnecessary and prevents semantic-stability
guarantees because the hook already returns a new object later; replace the
useMemo with a plain PageCta object (const cta: PageCta = { label:
t("shared.confirm"), onClick: () => onClickRef.current(), disabled:
kycGateIsBlocking, isLoading: unstakeIsLoading || yieldKycGate.isLoading }) so
you return a regular value and preserve the same referenced symbols (cta,
PageCta, onClickRef, kycGateIsBlocking, unstakeIsLoading,
yieldKycGate.isLoading) without using useMemo/useCallback.
In `@packages/widget/src/pages/steps/hooks/use-steps.hook.ts`:
- Around line 175-186: The CTA currently wrapped in useMemo (const cta =
useMemo<PageCta>(...)) should be converted to a plain derived value: remove the
useMemo import/usage and return a direct PageCta (or null) based on
txStates.length, preserving the same shape (disabled, isLoading, label:
t("shared.cancel"), onClick calling onClickRef.current(), variant: "secondary")
and keeping the PageCta type annotation; this drops unnecessary identity
memoization while retaining onClickRef, txStates and t usage.
🪄 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: CHILL
Plan: Pro
Run ID: 6c685530-9601-45ef-aa14-02bc6b706101
📒 Files selected for processing (37)
packages/widget/src/Widget.tsxpackages/widget/src/domain/types/yields.tspackages/widget/src/hooks/api/use-dashboard-yield-catalog.tspackages/widget/src/navigation/containers/animation-layout.tsxpackages/widget/src/navigation/containers/animation-page.tsxpackages/widget/src/pages-dashboard/activity/activity-details.page.tsxpackages/widget/src/pages-dashboard/common/components/footer-outlet/index.tsxpackages/widget/src/pages-dashboard/common/components/tabs/index.tsxpackages/widget/src/pages-dashboard/overview/earn-page/index.tsxpackages/widget/src/pages-dashboard/overview/index.tsxpackages/widget/src/pages-dashboard/position-details/components/position-details-actions.tsxpackages/widget/src/pages-dashboard/position-details/components/position-details-stake-actions.tsxpackages/widget/src/pages-dashboard/position-details/index.tsxpackages/widget/src/pages/complete/hooks/use-complete.hook.tspackages/widget/src/pages/complete/pages/common.page.tsxpackages/widget/src/pages/complete/state/index.tsxpackages/widget/src/pages/components/footer-outlet/context.tspackages/widget/src/pages/components/footer-outlet/index.tsxpackages/widget/src/pages/components/footer-outlet/styles.css.tspackages/widget/src/pages/components/page-cta.tsxpackages/widget/src/pages/details/earn-page/earn.page.tsxpackages/widget/src/pages/details/earn-page/state/earn-page-context.tsxpackages/widget/src/pages/details/earn-page/state/earn-page-state-context.tsxpackages/widget/src/pages/details/earn-page/state/types.tspackages/widget/src/pages/review/hooks/use-action-review.hook.tspackages/widget/src/pages/review/hooks/use-pending-review.hook.tspackages/widget/src/pages/review/hooks/use-stake-review.hook.tspackages/widget/src/pages/review/hooks/use-unstake-review.hook.tspackages/widget/src/pages/review/pages/action-review.page.tsxpackages/widget/src/pages/review/pages/common-page/common.page.tsxpackages/widget/src/pages/review/pages/pending-review.page.tsxpackages/widget/src/pages/review/pages/stake-review.page.tsxpackages/widget/src/pages/review/pages/unstake-review.page.tsxpackages/widget/src/pages/steps/hooks/use-steps.hook.tspackages/widget/src/pages/steps/pages/common.page.tsxpackages/widget/src/providers/index.tsxpackages/widget/tests/pages-dashboard/position-details-action-tabs.test.tsx
💤 Files with no reviewable changes (8)
- packages/widget/src/pages/components/footer-outlet/index.tsx
- packages/widget/src/Widget.tsx
- packages/widget/src/pages/components/footer-outlet/context.ts
- packages/widget/src/pages-dashboard/overview/index.tsx
- packages/widget/src/pages/components/footer-outlet/styles.css.ts
- packages/widget/src/pages-dashboard/position-details/index.tsx
- packages/widget/src/pages-dashboard/common/components/footer-outlet/index.tsx
- packages/widget/tests/pages-dashboard/position-details-action-tabs.test.tsx
| const onTokenBalanceSelect = useCallback( | ||
| async (tokenBalance: TokenBalanceScanResponseDto) => { | ||
| const category = dashboardVariant ? selectedDashboardYieldCategory : null; | ||
|
|
||
| if (!category) { | ||
| dispatch({ type: "token/select", data: tokenBalance.token }); | ||
| return; | ||
| } | ||
|
|
||
| const selectionRequestId = dashboardTokenSelectionRequestRef.current + 1; | ||
| dashboardTokenSelectionRequestRef.current = selectionRequestId; | ||
| dispatch({ type: "token-only/select", data: tokenBalance.token }); | ||
|
|
||
| const tokenYield = await getDashboardTokenYield({ | ||
| category, | ||
| tokenBalance, | ||
| }).catch((error) => { | ||
| console.log(error); | ||
| return null; | ||
| }); | ||
|
|
||
| if ( | ||
| selectionRequestId !== dashboardTokenSelectionRequestRef.current || | ||
| !tokenYield | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| selectDashboardTokenYield({ | ||
| token: tokenBalance.token, | ||
| yieldDto: tokenYield.yieldDto, | ||
| yieldId: tokenYield.yieldId, | ||
| }); | ||
| }, | ||
| [ | ||
| dashboardVariant, | ||
| dispatch, | ||
| getDashboardTokenYield, | ||
| selectedDashboardYieldCategory, | ||
| selectDashboardTokenYield, | ||
| ] | ||
| (tokenBalance: TokenBalanceScanResponseDto) => | ||
| dispatch({ type: "token/select", data: tokenBalance.token }), | ||
| [dispatch] | ||
| ); |
There was a problem hiding this comment.
Keep token picks category-aware on dashboard flows.
This now routes every token click through "token/select". That reducer path recomputes the initial yield from the token alone, so picking a token while the user is on rwa or defi can auto-select a yield from another category and immediately change selectedDashboardYieldCategory to that new yield's category. Route dashboard token picks through the category-aware selection path when a dashboard category is active.
🤖 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 `@packages/widget/src/pages/details/earn-page/state/earn-page-context.tsx`
around lines 476 - 480, onTokenBalanceSelect currently always dispatches { type:
"token/select" } which ignores dashboard context and can change
selectedDashboardYieldCategory; instead, read selectedDashboardYieldCategory
from state (or props) and when a dashboard category is active dispatch the
category-aware action (e.g. { type: "dashboard/token/select", data: { token:
tokenBalance.token, category: selectedDashboardYieldCategory } } or the existing
category-aware action your reducer expects) otherwise fall back to dispatching {
type: "token/select", data: tokenBalance.token }; update the
onTokenBalanceSelect useCallback to branch on selectedDashboardYieldCategory and
call the appropriate dispatch so token picks on dashboard flows preserve the
active category.
Move stake initialization into the earn page reducer. Skip catalog probes while wallet connection is pending.
3be9f08 to
5f2d2d1
Compare
| selectedDashboardYieldCategory, | ||
| selectDashboardTokenYield, | ||
| ] | ||
| (tokenBalance: TokenBalanceScanResponseDto) => |
There was a problem hiding this comment.
possible dashboard regression? token selection no longer preserves the active category and can fall back to a yield from a different tab
| @@ -684,12 +491,14 @@ export const EarnPageContextProvider = ({ | |||
| setSelectedDashboardYieldCategoryFallback(category); | |||
|
|
|||
| const target = | |||
There was a problem hiding this comment.
same here: switching back to a category now restores its initial seed instead of the last token/yield previously selected in that tab
Summary by CodeRabbit
Refactor
New Features
Other