Conversation
🦋 Changeset detectedLatest commit: d0efe01 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughPaymentSheet was refactored from an externally controlled modal to an internally managed component that syncs maturity from the URL and renders multiple internal views. Home/Loans/PayMode stop toggling the sheet and now set router params. Overdue/Upcoming onSelect signatures now accept only maturity. Minor DeFi copy and one Spanish i18n key added. Three changeset files were added. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Home / Loans / PayMode
participant Router as Router
participant PaymentSheet as PaymentSheet
participant Views as InternalViews
participant Nav as Navigation
App->>Router: setParams(maturity)
Router->>PaymentSheet: render (URL includes maturity)
PaymentSheet->>PaymentSheet: sync params → set displayMaturity & open
PaymentSheet->>PaymentSheet: compute borrow preview (useMemo)
PaymentSheet->>Views: renderContent → NotAvailable / RolloverIntro / Details
Views->>PaymentSheet: user action (confirm / rollover / close)
PaymentSheet->>Nav: navigateToRepay / navigateToRollover
Nav->>Router: navigate to /pay or /roll-debt (clear maturity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
Summary of ChangesHello @dieguezguille, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the payment flow by refactoring the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several miscellaneous fixes and a significant refactoring of the payment modal. The PaymentSheet component is now self-contained, managing its own visibility based on URL parameters, which simplifies parent components like Home, Loans, and PayMode. The UI for DeFi service buttons has also been improved to show dynamic descriptions based on connection status. However, a critical security concern has been identified in src/components/pay-mode/PaymentSheet.tsx: the removal of input validation for the maturity URL parameter could lead to application crashes when processing malformed input from deep links. Additionally, a code quality improvement is suggested to avoid side-effects during rendering. Please see the detailed comments for more information.
| previewValue: bigint; | ||
| } | ||
| >(() => { | ||
| if (!currentMaturity || !USDCMarket) return; |
There was a problem hiding this comment.
The validation for the maturity parameter from the URL has been removed. The previous implementation used valibot to ensure maturity is a valid digit string. Now, BigInt(currentMaturity) is called directly on line 301, which will throw an unhandled exception if currentMaturity is not a valid integer string (e.g., an empty string or any non-numeric value), causing the component to crash. It's important to reintroduce validation to prevent this. A simple regex check can ensure currentMaturity is a string of digits before attempting to convert it to a BigInt.
| if (!currentMaturity || !USDCMarket) return; | |
| if (!currentMaturity || !/^\d+$/.test(currentMaturity) || !USDCMarket) return; |
| >(() => { | ||
| if (!currentMaturity || !USDCMarket) return; | ||
| const { fixedBorrowPositions, usdPrice, decimals } = USDCMarket; | ||
| const position = fixedBorrowPositions.find((b) => b.maturity === BigInt(currentMaturity)); |
There was a problem hiding this comment.
The maturity parameter is retrieved directly from URL parameters via useLocalSearchParams and used in BigInt(currentMaturity) on line 301 and Number(currentMaturity) on line 306 without validation. If an attacker provides a non-numeric string (e.g., via a deep link), BigInt() will throw a SyntaxError, causing the component to fail to render and potentially crashing the application. This is a regression from the previous version which used valibot for validation.
| const position = fixedBorrowPositions.find((b) => b.maturity === BigInt(currentMaturity)); | |
| const position = /^\d+$/.test(currentMaturity) ? fixedBorrowPositions.find((b) => b.maturity === BigInt(currentMaturity)) : undefined; |
| const displayMaturityRef = useRef(maturity); | ||
| if (maturity) displayMaturityRef.current = maturity; |
There was a problem hiding this comment.
Mutating a ref directly in the render body is a side-effect and is generally discouraged. To persist the maturity value for display purposes even after the URL parameter is removed, it's cleaner to use useState and useEffect. This avoids side-effects during rendering and makes the component's state flow more predictable.
After this change, you'll need to replace all instances of displayMaturityRef.current with the new state variable (e.g., displayMaturity).
| const displayMaturityRef = useRef(maturity); | |
| if (maturity) displayMaturityRef.current = maturity; | |
| const [displayMaturity, setDisplayMaturity] = useState(maturity); | |
| useEffect(() => { | |
| if (maturity) setDisplayMaturity(maturity); | |
| }, [maturity]); |
|
@cursor review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/defi/DeFi.tsx`:
- Around line 83-85: The JSX prop expression for description in DeFi.tsx is
split across multiple lines and needs to be collapsed into a single-line
expression to satisfy Prettier; replace the multi-line ternary using
lifiConnected and t with a single-line prop assignment (i.e.,
description={lifiConnected ? t("Disconnect your wallet from LI.FI") : t("Connect
your wallet to LI.FI")} ) so the description prop is one line.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #718 +/- ##
==========================================
+ Coverage 68.23% 68.58% +0.34%
==========================================
Files 199 199
Lines 6357 6443 +86
Branches 1870 1901 +31
==========================================
+ Hits 4338 4419 +81
Misses 1837 1837
- Partials 182 187 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/defi/DeFi.tsx`:
- Around line 62-66: The JSX prop value for description in the DeFi component is
split across multiple lines while similar props (e.g., the LI.FI description)
are single-line; collapse the ternary expression into a single line to match
style and avoid voluntary line breaks — update the description prop that uses
fundingConnected ? t("Disconnect your wallet from Exactly Protocol") :
t("Connect your wallet to Exactly Protocol") so it fits on one line within the
DeFi component (maintain the same variables and t(...) calls).
In `@src/components/pay-mode/PaymentSheet.tsx`:
- Around line 282-287: In the PaymentSheet component's useEffect (the one that
calls setDisplayMaturity and setOpen), remove open from the dependency array so
the effect depends only on maturity; keep the existing conditional checks and
the eslint-disable comments on setDisplayMaturity and setOpen to avoid linter
errors, i.e. change the dependency list from [maturity, open] to [maturity]
while leaving the body (if (maturity) { setDisplayMaturity(maturity); if (!open)
setOpen(true); }) intact so the openness logic still guards against unnecessary
state changes.
| useEffect(() => { | ||
| if (maturity) { | ||
| setDisplayMaturity(maturity); // eslint-disable-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect | ||
| if (!open) setOpen(true); // eslint-disable-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect | ||
| } | ||
| }, [maturity, open]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify effect doesn't re-open on every render.
The open dependency could cause unexpected behavior. When maturity is set and open becomes true, the effect runs again because open changed. The condition if (!open) setOpen(true) prevents infinite loops, but open in the dependency array seems unnecessary since maturity alone should drive the open state.
Consider removing open from dependencies:
♻️ Suggested refinement
useEffect(() => {
if (maturity) {
setDisplayMaturity(maturity); // eslint-disable-line `@eslint-react/hooks-extra/no-direct-set-state-in-use-effect`
if (!open) setOpen(true); // eslint-disable-line `@eslint-react/hooks-extra/no-direct-set-state-in-use-effect`
}
- }, [maturity, open]);
+ }, [maturity]); // eslint-disable-line react-hooks/exhaustive-deps📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (maturity) { | |
| setDisplayMaturity(maturity); // eslint-disable-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect | |
| if (!open) setOpen(true); // eslint-disable-line @eslint-react/hooks-extra/no-direct-set-state-in-use-effect | |
| } | |
| }, [maturity, open]); | |
| useEffect(() => { | |
| if (maturity) { | |
| setDisplayMaturity(maturity); // eslint-disable-line `@eslint-react/hooks-extra/no-direct-set-state-in-use-effect` | |
| if (!open) setOpen(true); // eslint-disable-line `@eslint-react/hooks-extra/no-direct-set-state-in-use-effect` | |
| } | |
| }, [maturity]); // eslint-disable-line react-hooks/exhaustive-deps |
🤖 Prompt for AI Agents
In `@src/components/pay-mode/PaymentSheet.tsx` around lines 282 - 287, In the
PaymentSheet component's useEffect (the one that calls setDisplayMaturity and
setOpen), remove open from the dependency array so the effect depends only on
maturity; keep the existing conditional checks and the eslint-disable comments
on setDisplayMaturity and setOpen to avoid linter errors, i.e. change the
dependency list from [maturity, open] to [maturity] while leaving the body (if
(maturity) { setDisplayMaturity(maturity); if (!open) setOpen(true); }) intact
so the openness logic still guards against unnecessary state changes.
dieguezguille
force-pushed
the
fixes
branch
4 times, most recently
from
68f94d6 to
52b13d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/pay-mode/PaymentSheet.tsx`:
- Around line 347-367: The callbacks navigateToRepay and navigateToRollover
assume displayMaturity is present due to render gating; add an explicit guard at
the start of each callback (e.g., if (!displayMaturity) return) so they bail out
early when displayMaturity is undefined/null before calling router.navigate or
using it in params, and update dependency/composition if needed to keep
type-safety for displayMaturity in those functions.
- Around line 144-256: The UI currently risks truncating fractional token
amounts if any code (e.g., in DetailsView) uses BigInt division like
previewValue / BigInt(10 ** decimals); instead, locate uses of previewValue and
positionValue in DetailsView and convert for display by computing
Number(previewValue) / 10 ** decimals (and Number(positionValue) / 10 **
decimals) at render time, using the correct market decimals variable instead of
performing BigInt division, so fractional precision is preserved in the
displayed amounts and discount calculations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/pay-mode/PaymentSheet.tsx`:
- Around line 284-285: The eslint-disable-line annotations on the direct state
sets for setDisplayMaturity and setOpen need a brutally concise, lowercase
rationale; update the two comments that follow setDisplayMaturity(maturity) and
if (!open) setOpen(true) to append a short lowercase reason (e.g., "direct set
during effect" or "sync initial state in effect") so the comments read as
eslint-disable-line ... // reason, keeping the text lowercase and very brief.
cruzdanilo
force-pushed
the
fixes
branch
2 times, most recently
from
a7e62cc to
9431b91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/pay-mode/PaymentSheet.tsx`:
- Around line 80-83: The CalendarImage SVG is being passed string percentage
values for width/height inside PaymentSheet (see the CalendarImage usage in the
View wrapper); change those props to numeric values (either fixed numbers or
computed numeric expressions from layout/Dimensions/onLayout) so the
react-native-svg component receives numbers, e.g., compute the desired pixel
width/height from the parent container and pass those numeric values into
CalendarImage; update the CalendarImage usage in PaymentSheet.tsx and any
onLayout/size calculation helper used to produce the numeric width/height.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/home/Home.tsx`:
- Around line 192-193: The onSelect callbacks for OverduePayments and
UpcomingPayments currently accept (maturity, amount) from the child but only use
maturity, which discards amount; update the inline handlers to explicitly accept
both parameters and either ignore amount by naming it _ or _amount (e.g.,
onSelect={(m, _amount) => router.setParams({ maturity: String(m) })}) or, if the
amount should be preserved, include it in the call to router.setParams (e.g.,
router.setParams({ maturity: String(m), amount: String(_amount) })); modify the
two occurrences (OverduePayments and UpcomingPayments onSelect props)
accordingly to make the unused parameter intentional or to forward it.
In `@src/components/loans/Loans.tsx`:
- Line 84: The onSelect callback passed to UpcomingPayments currently accepts
only the first argument and drops the second (amount); update the handler used
in Loans.tsx so it matches the typed signature (maturity: bigint, amount:
bigint) — e.g. declare parameters (maturity, _amount) or (maturity, amount) and
then call router.setParams({ maturity: String(maturity) }) (or use amount if
intended) so the second parameter is explicitly acknowledged and no argument is
silently discarded; reference UpcomingPayments and router.setParams when making
the change.
In `@src/components/pay-mode/PayMode.tsx`:
- Around line 58-59: The onSelect callbacks for OverduePayments and
UpcomingPayments currently accept only the maturity argument and drop the second
amount parameter; update the handlers to match the declared signature by
accepting both parameters (e.g., maturity and amount) and explicitly ignore the
amount (use an underscore-prefixed name) while still calling router.setParams({
maturity: String(maturity) }) so the signature is consistent; locate the
onSelect props on OverduePayments and UpcomingPayments in PayMode.tsx and adjust
the arrow functions accordingly.
| setPaySheetOpen(true); | ||
| }} | ||
| /> | ||
| <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Callback signature discards the amount parameter.
Per UpcomingPayments.tsx, onSelect is typed as (maturity: bigint, amount: bigint) => void. The callback (m) => router.setParams(...) accepts only one argument, silently discarding amount. If intentional, consider explicitly acknowledging the unused parameter: (m, _amount).
♻️ Suggested fix
- <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} />
+ <UpcomingPayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> | |
| <UpcomingPayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} /> |
🤖 Prompt for AI Agents
In `@src/components/loans/Loans.tsx` at line 84, The onSelect callback passed to
UpcomingPayments currently accepts only the first argument and drops the second
(amount); update the handler used in Loans.tsx so it matches the typed signature
(maturity: bigint, amount: bigint) — e.g. declare parameters (maturity, _amount)
or (maturity, amount) and then call router.setParams({ maturity:
String(maturity) }) (or use amount if intended) so the second parameter is
explicitly acknowledged and no argument is silently discarded; reference
UpcomingPayments and router.setParams when making the change.
| <OverduePayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> | ||
| <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Callback signatures discard the amount parameter.
Same pattern as other files—onSelect receives (maturity, amount) but only maturity is used. Consider acknowledging the unused parameter for consistency.
♻️ Suggested fix
- <OverduePayments onSelect={(m) => router.setParams({ maturity: String(m) })} />
- <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} />
+ <OverduePayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} />
+ <UpcomingPayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <OverduePayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> | |
| <UpcomingPayments onSelect={(m) => router.setParams({ maturity: String(m) })} /> | |
| <OverduePayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} /> | |
| <UpcomingPayments onSelect={(m, _) => router.setParams({ maturity: String(m) })} /> |
🤖 Prompt for AI Agents
In `@src/components/pay-mode/PayMode.tsx` around lines 58 - 59, The onSelect
callbacks for OverduePayments and UpcomingPayments currently accept only the
maturity argument and drop the second amount parameter; update the handlers to
match the declared signature by accepting both parameters (e.g., maturity and
amount) and explicitly ignore the amount (use an underscore-prefixed name) while
still calling router.setParams({ maturity: String(maturity) }) so the signature
is consistent; locate the onSelect props on OverduePayments and UpcomingPayments
in PayMode.tsx and adjust the arrow functions accordingly.
3ad751b to
e63a77f
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
Summary by CodeRabbit
Improvements
Bug Fixes
Localization
Chores