refactor: remove derived-state useEffect/useMemo patterns#830
refactor: remove derived-state useEffect/useMemo patterns#830
useEffect/useMemo patterns#830Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRemoves unnecessary useMemo wrappers across several hooks and components, converts token balance state to a memoized computation, and adds comprehensive test suites for four hooks. Changes preserve external behavior and exported signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (5)
packages/web/src/layouts/pool/pool-detail/components/staking/staking-content/staking-content-card/StakingContentCard.tsx (1)
114-116: Residual trivialuseMemoforperiodInfo— same pattern as the removedcheckedStep.Both
StakingContentCard(Lines 114–116) andSummuryApr(Lines 291–293) still wrap a plain object property access inuseMemo:const periodInfo = useMemo(() => { return STAKING_PERIOD_INFO[period]; }, [period]);
STAKING_PERIOD_INFO[period]is a synchronous constant lookup with no side-effects, making this identical in nature to thecheckPoints.includes(period)pattern just removed.♻️ Proposed simplification (applies to both components)
- const periodInfo = useMemo(() => { - return STAKING_PERIOD_INFO[period]; - }, [period]); + const periodInfo = STAKING_PERIOD_INFO[period];packages/web/src/hooks/token/data/use-token-amount-input.spec.tsx (1)
27-42: Preferas unknown as TokenModeloveras neverfor themakeTokenhelper return type.
neveris the TypeScript bottom type (a value that can never exist), so casting a real object to it is semantically inverted.as unknown as TokenModel(or simply typing the return: TokenModel) gives the correct type-narrowing intent and prevents accidental misuse in future tests.🛠️ Proposed fix
+import { TokenModel } from "@models/token/token-model"; + -function makeToken(path: string, decimals = 6) { +function makeToken(path: string, decimals = 6): TokenModel { return { path, ... - } as never; + } as unknown as TokenModel; }packages/web/src/hooks/pool/data/use-decrease-handle.spec.tsx (1)
107-125: Consider addinginRange/rangeStatustest cases — the refactored logic is currently uncovered.The
inRangecomputation (use-decrease-handle.tsxLines 126–129) was refactored fromuseMemoto a direct boolean in this PR, but no test verifies its IN / OUT / NONE outcomes. ThemakePositionhelper already provides everything needed.💡 Example cases to add
+ describe("rangeStatus", () => { + it("returns IN when currentTick is within [tickLower, tickUpper]", () => { + mockSelectedPosition = makePosition(100, 50, 150); + const { result } = renderHook(() => useDecreaseHandle(), { wrapper }); + expect(result.current.rangeStatus).toBe(RANGE_STATUS_OPTION.IN); + }); + + it("returns OUT when currentTick is below tickLower", () => { + mockSelectedPosition = makePosition(30, 50, 150); + const { result } = renderHook(() => useDecreaseHandle(), { wrapper }); + expect(result.current.rangeStatus).toBe(RANGE_STATUS_OPTION.OUT); + }); + + it("returns NONE when position is closed", () => { + mockSelectedPosition = makePosition(100, 50, 150, true); + const { result } = renderHook(() => useDecreaseHandle(), { wrapper }); + expect(result.current.rangeStatus).toBe(RANGE_STATUS_OPTION.NONE); + }); + });packages/web/src/hooks/common/use-referral.spec.tsx (1)
45-73: WraprenderHookcalls inJotaiProviderandGnoswapThemeProvider.All five
renderHookinvocations are missing the required providerwrapper. Even thoughuseReferral's atom-consuming dependencies are mocked here, the codebase-wide rule still applies and guards against test failures if future hooks added touseReferralpull from theme or Jotai context.♻️ Proposed fix — add a shared wrapper
+import { JotaiProvider } from "@/common/providers/jotai-provider"; +import { GnoswapThemeProvider } from "@/common/providers/gnoswap-theme-provider"; + +const wrapper = ({ children }: { children: React.ReactNode }) => ( + <JotaiProvider> + <GnoswapThemeProvider> + {children} + </GnoswapThemeProvider> + </JotaiProvider> +); it("returns 0 when leaderboardMyInfo is undefined", () => { mockLeaderboardMyInfo = undefined; - const { result } = renderHook(() => useReferral()); + const { result } = renderHook(() => useReferral(), { wrapper }); expect(result.current.referralEarnedPoints).toBe(0); });Apply the same
{ wrapper }option to the remaining fourrenderHookcalls.Based on learnings: "Applies to
**/*.spec.{ts,tsx}: Wrap components inJotaiProviderandGnoswapThemeProviderwhen testing with testing-library/react."packages/web/src/hooks/pool/data/use-increase-handle.spec.tsx (1)
92-128: WraprenderHookcalls inJotaiProviderandGnoswapThemeProvider.None of the five
renderHookinvocations supply awrapper. While the blanket Jotai mock removes the runtime need forJotaiProviderin these specific tests, the guideline is unconditional for this codebase.♻️ Proposed fix — add a shared wrapper
+import React from "react"; +import { JotaiProvider } from "@/common/providers/jotai-provider"; +import { GnoswapThemeProvider } from "@/common/providers/gnoswap-theme-provider"; + +const wrapper = ({ children }: { children: React.ReactNode }) => ( + <JotaiProvider> + <GnoswapThemeProvider> + {children} + </GnoswapThemeProvider> + </JotaiProvider> +); it("returns false when selectedPosition is null", () => { mockSelectedPosition = null; - const { result } = renderHook(() => useIncreaseHandle()); + const { result } = renderHook(() => useIncreaseHandle(), { wrapper }); expect(result.current.loading).toBe(true); });Apply the same
{ wrapper }option to the remaining fourrenderHookcalls.Based on learnings: "Applies to
**/*.spec.{ts,tsx}: Wrap components inJotaiProviderandGnoswapThemeProviderwhen testing with testing-library/react."
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/web/src/components/common/line-graph/LineGraph.tsxpackages/web/src/hooks/common/use-referral.spec.tsxpackages/web/src/hooks/common/use-referral.tsxpackages/web/src/hooks/pool/data/use-decrease-handle.spec.tsxpackages/web/src/hooks/pool/data/use-decrease-handle.tsxpackages/web/src/hooks/pool/data/use-increase-handle.spec.tsxpackages/web/src/hooks/pool/data/use-increase-handle.tsxpackages/web/src/hooks/pool/data/use-select-pool.tsxpackages/web/src/hooks/token/data/use-token-amount-input.spec.tsxpackages/web/src/hooks/token/data/use-token-amount-input.tsxpackages/web/src/layouts/pool/pool-detail/components/staking/staking-content/staking-content-card/StakingContentCard.tsx
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/web/src/hooks/token/data/use-token-amount-input.spec.tsx (1)
37-52:⚠️ Potential issue | 🔴 CriticalRemove
as nevercast and fix type mismatches inmakeToken.The
as nevercast suppresses type safety and hides actual type errors in the fixture. The object has two problems:
type: 0should betype: "Native"ortype: "GRC20"(TokenModel expects a string, not a number)isGnot: falsedoes not exist in TokenModel and should be removedReplace the unsafe cast with an explicit return type derived from the hook signature:
Suggested fix
+type HookToken = Exclude<Parameters<typeof useTokenAmountInput>[0], null>; + -function makeToken(path: string, decimals = 6) { - return { +function makeToken(path: string, decimals = 6): HookToken { + const token: HookToken = { path, name: "TestToken", symbol: "TT", decimals, logoURI: "", priceID: path, chainId: "test", createdAt: "", address: "", wrappedPath: path, - type: 0, - isGnot: false, + type: "Native", - } as never; + }; + return token; }
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/src/hooks/pool/data/use-decrease-handle.spec.tsxpackages/web/src/hooks/token/data/use-token-amount-input.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/hooks/pool/data/use-decrease-handle.spec.tsx
1 participant
Description
When a value is purely derived from props or other state, synchronizing it through
useState+useEffectcauses two render cycles per change.Changes
useDecreaseHandlewhereselectedPositionwas duplicated in the useEffect deps, which could mask stale closure issuesuseState+useEffectsync patternuseMemowrappers where the computation is a simple property access, boolean check, or constantuseMemoidentity wrapper inuseSelectPool(poolPaththat just returnedlatestPoolPath)Summary by CodeRabbit
Tests
Refactor