refactor(swap): model confirm modal state as discriminated union#859
refactor(swap): model confirm modal state as discriminated union#859
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe changes refactor swap confirmation modal state management from a nullable-field interface to a discriminated union type with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 docstrings
🧪 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: 2
🧹 Nitpick comments (1)
packages/web/src/components/swap/confirm-swap-modal/ConfirmSwapModal.spec.tsx (1)
4-4: Consider extractingSwapRateActionout of the hook module.Importing
SwapRateActionfrom@hooks/swap/data/use-swap-handlerpulls the entireuseSwapHandlermodule graph (includingConfirmSwapModal, query hooks, broadcast handlers, etc.) into the test file just to get an enum value. This bloats the test compilation/eval graph and creates a minor circular import risk (hook importsConfirmSwapModal; this spec testsConfirmSwapModaland imports from the hook).Consider moving
SwapRateAction(andPriceImpactStatus) to a pure types module like@models/swap/swap-rate-action.tsand re-exporting from the hook. The production component and this test can then import the enum without dragging the whole hook along.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b05bb414-2892-48f6-8111-105ff5713c62
📒 Files selected for processing (4)
packages/web/src/components/swap/confirm-swap-modal/ConfirmSwapModal.spec.tsxpackages/web/src/components/swap/confirm-swap-modal/ConfirmSwapModal.tsxpackages/web/src/hooks/swap/data/use-swap-handler.tsxpackages/web/src/states/swap.ts
| it("does not render when confirm modal state is idle", () => { | ||
| const { container } = renderModal({ status: "idle" }); | ||
|
|
||
| expect(container.firstChild).toBeNull(); | ||
| expect(screen.queryByText("Confirm swap")).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'GnoswapThemeProvider' --exec cat {}Repository: gnoswap-labs/gnoswap-interface
Length of output: 1871
Update test assertion — container.firstChild will not be null with GnoswapThemeProvider.
The idle-case assertion expect(container.firstChild).toBeNull() will fail because GnoswapThemeProvider renders a <Global> component that produces DOM nodes, even when ConfirmSwapModal correctly returns null. Since wrapping components in GnoswapThemeProvider is required for tests, replace this assertion with expect(screen.queryByText("Confirm swap")).not.toBeInTheDocument(), which already provides the correct functional coverage and aligns with the provider's DOM output.
| useEffect(() => { | ||
| if (!swapTokenInfo || !swapSummaryInfo) return; | ||
|
|
||
| setSwapConfirmModalState(prev => ({ | ||
| ...prev, | ||
| setSwapConfirmModalState({ | ||
| status: "ready", | ||
| swapTokenInfo, | ||
| swapSummaryInfo, | ||
| isRefetching, | ||
| estimatedAmount, | ||
| tokenAmountLimit, | ||
| })); | ||
| }); | ||
| }, [swapTokenInfo, swapSummaryInfo, isRefetching, estimatedAmount, tokenAmountLimit]); |
There was a problem hiding this comment.
State transition is one-way: "idle" → "ready" only.
Once set to "ready", the atom is never reset back to "idle" from this hook, even when tokenA/tokenB are cleared or swapSummaryInfo becomes null again. The PR description calls this out as intentional ("Preserve existing state-transition semantics"), so behavior matches the previous nullable model.
One caveat to be aware of: because swapConfirmModalState is a module-level atom (not scoped to the modal lifetime), stale swapTokenInfo/swapSummaryInfo from a prior session can remain in the atom until the next render that has fresh values. If the modal is reopened synchronously with different tokens, the wrapper in ConfirmSwapModal.tsx will render the old "ready" snapshot for one frame before the effect runs. If that flicker is acceptable (and it seemingly was before this refactor), no action needed — otherwise, consider resetting to { status: "idle" } in the else branch or on closeModal.
1 participant
Description
SwapConfirmModalStatefrom a nullable object shape to a discriminated union withidleandreadystatesConfirmSwapModalat the component boundary so ready-only data is handled as non-null inside the rendered contentI did not change the state transition to force
{ status: "idle" }when modal inputs become unavailable. That behavior was potentially ambiguous, so the existing semantics were preserved.Summary by CodeRabbit
Tests
Refactor