257: Extend refinance to migrate collateral#575
Merged
Conversation
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
|
🚅 Deployed to the euler-lite-pr-575 environment in euler-lite
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the standalone collateral-swap page with a unified refinance flow at ChangesUnified Refinance Flow
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as borrow/swap.vue
participant CollOpts as useSwapCollateralOptions
participant DebtOpts as useSwapDebtOptions
participant EulerTx as useEulerTx
participant SDK as sdk.executionService
participant Modal as OperationReviewModal
User->>Page: navigate with ?collateral=0x...
Page->>CollOpts: allCollateralOptions (includes compatibilityWarning)
Page->>DebtOpts: allBorrowOptions (allBorrowVaults)
User->>Page: select target collateral and/or debt vault
Page->>Page: fetch collateral + debt swap quotes in parallel
User->>Page: click "Review Refinance"
Page->>EulerTx: planRefinancePosition({ collateral, debt })
EulerTx->>SDK: planSwapCollateral or planMigrateSameAssetCollateral
EulerTx->>SDK: planSwapDebt or planMigrateSameAssetDebt
SDK-->>EulerTx: sub-plans
EulerTx->>SDK: mergePlans([collateralPlan, debtPlan])
SDK-->>EulerTx: TransactionPlan
EulerTx-->>Page: TransactionPlan
Page->>Modal: open (type='refinance')
User->>Modal: confirm
Modal->>EulerTx: execute TransactionPlan
EulerTx-->>Page: success → refresh + redirect
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🤖 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 `@composables/useSwapDebtOptions.ts`:
- Around line 75-79: The allBorrowOptions reactive map is missing
collateralVault from its dependency array, even though the buildBorrowOption
function uses collateralVault?.value?.address to build borrow options with the
correct collateral context. When the selected collateral changes but the other
dependencies (viewer, enableIntrinsicApy, enableRewardsApy) remain the same,
allBorrowOptions will not recompute and will continue to use stale APY data from
the previous collateral context. Add collateralVault to the dependencies array
passed to useReactiveMap at lines 75-79 to ensure the options recompute whenever
the collateral changes.
In `@tests/golden/tx-plan-builders.test.ts`:
- Around line 548-560: The test's findIndex calls for enableCollateralIndex and
enableControllerIndex currently only match on selector and targetContract,
without verifying the decoded function arguments. This means if another
enableCollateral or enableController call with different arguments is added
earlier in the batch, the test could still pass even if the refinance-specific
order regresses. Enhance both findIndex predicates to also decode the call
arguments and verify they match the expected values: the enableCollateralIndex
should match calls with arguments (ADDR.subAccount1, ADDR.vaultWeth), and the
enableControllerIndex should match calls with arguments (ADDR.subAccount1,
ADDR.vaultUsdt).
In `@tests/utils/stepDecoding.test.ts`:
- Around line 101-103: The batchItem function in
tests/utils/stepDecoding.test.ts at lines 101-103 currently defaults
targetContract to verifier, which is not representative of real swap batches and
can mask target-aware decoding regressions. Change the default parameter or
remove it entirely to use an explicit swapper address instead of verifier. Apply
the same fix at the sibling locations at lines 180-184 and 196-199 where
batchItem is used in multicall fixture items, ensuring each call explicitly
passes a swapper targetContract rather than relying on the verifier default.
In `@utils/stepDecoding.ts`:
- Around line 299-308: The Withdraw label block incorrectly resolves the vault
asset by calling getVaultAssetInfo with the decoded calldata, which extracts the
receiver address from the second parameter of the withdraw/redeem function.
Since the receiver could be another known vault, this causes the wrong vault's
asset to display. Instead of using getVaultAssetInfo which parses the calldata
to get vault info, directly obtain the vault asset information from
targetContract (which represents the source vault) to ensure the asset displayed
always belongs to the source vault where the withdrawal originates, regardless
of the receiver's identity.
🪄 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: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20749a81-4afc-47d6-acfa-aeea6efd4c90
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
components/entities/operation/OperationReviewModal.vuecomponents/entities/vault/ChooseCollateralModal.vuecomposables/cowswap/index.tscomposables/useEulerTx.tscomposables/useSwapCollateralOptions.tscomposables/useSwapDebtOptions.tsdocs/geo-blocking.mdentities/cowswap/index.tspackage.jsonpages/position/[number]/borrow/swap.vuepages/position/[number]/collateral/swap.vuepages/position/[number]/index.vuetests/execution/groups.jsontests/execution/scenarios.jsontests/golden/tx-plan-builders.test.tstests/utils/stepDecoding.test.tstypes/collateral-option.tsutils/sdk-snapshot-populate-stubs.tsutils/stepDecoding.ts
💤 Files with no reviewable changes (3)
- pages/position/[number]/collateral/swap.vue
- tests/execution/scenarios.json
- tests/execution/groups.json
LeonardEulerXYZ
suggested changes
Jun 16, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 7983177.
Verdict: request changes. The combined refinance direction is sensible, but I found two blocking issues before the route is safe to merge.
Scope reviewed:
- position refinance route consolidation
- collateral/debt option compatibility handling
- CoW collateral-only path gating
- transaction review / step decoding updates
- dependency bump and lockfile health
Validation performed:
- inspected PR metadata, changed files, full diff, and surrounding route/composable context
- checked existing Leonard comments/reviews: none to update or clean up
- attempted clean install with the repo package manager path:
npm ci --ignore-scriptsfails becausepackage.jsonandpackage-lock.jsonare not in sync - attempted non-lock install for local validation: blocked by the repo
min-release-age=7policy for@eulerxyz/euler-v2-sdk@1.0.5 - checked PR status: Railway
dev-buildis failing
Smoke coverage: not run. Local install/bootstrap and the deployed preview build are both failing, so a browser/mobile smoke would not be meaningful yet. Screenshots were not posted for the same reason.
Findings are inline.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
dglowinski
force-pushed
the
euler-lite-migration
branch
from
6bd7fc2 to
71a82ed
Compare
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 71a82ed.
Verdict: comment — the two prior Leonard blockers I found are resolved on the current head, and I did not find a new blocking issue in this pass. I am not submitting approval because this review run was not explicitly scoped to approve.
What changed in the re-review:
- The collateral-row Refinance link now preserves the selected collateral as
?collateral=..., and the consolidated refinance page readsroute.query.collateral, selects the matching collateral position, and uses that position's assets/shares for the source side. - The lockfile/dependency bootstrap issue is resolved:
npm ci --ignore-scriptsnow passes. - I deleted the two stale Leonard inline comments that referred to those resolved issues.
Validation performed:
npm ci --ignore-scripts— passednpx nuxt prepare— passednpm run test:run -- tests/utils/stepDecoding.test.ts tests/utils/portfolio-borrow-position.test.ts tests/utils/position-roe.test.ts tests/composables/useMultiplyForm.test.ts tests/utils/token-categories.test.ts— passed, 20 testsnpm run typecheck— passednpm run build— passed- Headed Chromium under Xvfb, local production build:
- desktop
/position/1/borrow/swap?network=1rendered the Refinance route - mobile
/position/1/borrow/swap?network=1rendered the Refinance route - desktop
/position/1/borrow/swap?network=1&collateral=0x797DD80692c3b2dAdabCe8e30C07fDE5307D48a9rendered the Refinance route with the selected-collateral query accepted - legacy
/position/1/collateral/swap?network=1renders Nuxt Page not found after the route deletion
- desktop
Non-blocking caveat:
- The old
/position/:number/collateral/swaproute is now gone. If preserving old deep links/bookmarks is a product requirement, add a redirect/shim to the consolidated refinance route; otherwise this is just an intentional route migration.
Smoke coverage: browser visual smoke and mobile smoke over the consolidated refinance route; no wallet/signing smoke. I did not post screenshots because the smoke did not reveal a visual delta/finding that screenshots would clarify.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 16, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 6f38bf8.
Verdict: request changes. The core planner/test/build path is healthy, but I found one blocking URL-state bug in the consolidated refinance route, plus one smaller navigation regression to fix while touching this surface.
Scope reviewed:
- consolidated debt/collateral refinance route and planner inputs
- collateral/debt modal selection behavior and compatibility grouping
- removed legacy collateral swap route behavior
- latest incremental changes since my prior review (
ChooseCollateralModal,VaultForm, route back behavior)
Validation performed:
- inspected PR metadata, full diff, incremental diff since my prior review, prior Leonard reviews/comments, and surrounding route/composable context
npm run test:run -- tests/utils/stepDecoding.test.ts tests/utils/portfolio-borrow-position.test.ts tests/utils/position-roe.test.ts tests/composables/useMultiplyForm.test.ts tests/utils/token-categories.test.ts— passed, 20 testsnpm run typecheck— passednpm run build— passed- headed Chromium under Xvfb against the PR preview:
- desktop
/position/1/borrow/swap?network=1&collateral=...rendered the Refinance route - desktop invalid
collateral=...still rendered the Refinance route, matching the code-path concern below rather than rejecting/canonicalizing the stale source - mobile
/position/1/borrow/swap?network=1back navigation did not preserve the intended position route context - legacy
/position/1/collateral/swap?network=1renders Nuxt Page not found after route deletion
- desktop
Smoke coverage: browser route smoke and mobile back-navigation smoke; no wallet/signing smoke. Screenshots were captured locally but not posted because they only show generic route shells / 404 and do not add useful public evidence beyond the route results above.
Comment lifecycle: no Leonard top-level issue comment existed to edit; no current Leonard inline review comments were present to delete. This review supersedes my prior comment on head 71a82ed.
Filter refinance debt and collateral selections so each choice leaves a compatible counterpart, and keep the modal grouping explicit when no compatible vaults remain.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 16, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head b326003.
Verdict: request changes. The focused test/type/build path is healthy and the preview route renders, but the current head still has one transaction-state blocker from the prior review, plus one new route-state/selection-risk issue worth fixing before merge.
Findings:
- Blocking: the existing Leonard inline comment on
sourceCollateralVaultstill applies. A stale/craftedcollateral=query can resolve any known vault via the registry even when that vault is not one of the position's collaterals; the amount then falls back to the position's primary supplied amount. That can make the refinance planner/UI pair the wrong source vault with the wrong source balance. - Blocking/risk: modal selection is still index-based while options are produced asynchronously separately from the vault arrays. If the displayed options lag the vault arrays during a debt/collateral context change, a row click can select a different vault than the user clicked.
- Navigation gap: the newly generated Refinance links/back fallbacks should preserve the active query state, especially
network, and the deleted/position/:number/collateral/swaproute has no compatibility redirect to the consolidated route.
Validation performed:
- inspected PR metadata, body, changed files, full diff, incremental diff since the prior Leonard review, and surrounding route/composable/modal context
- inspected existing Leonard issue comments, formal reviews, and inline comments for PR #575
npm run test:run -- tests/utils/stepDecoding.test.tspassednpm run typecheckpassednpm run buildpassed- headed Chromium under Xvfb mobile route smoke: PR preview and development comparator both loaded
/position/1/borrow/swap?network=1with HTTP 200 and no page errors; PR rendered the consolidatedRefinanceroute, development rendered the oldRefinance debtroute
Smoke coverage: mobile browser route smoke only; no wallet/signing coverage. Screenshots were not posted because the findings are URL/state and planner-input issues, not visual-only deltas.
Comment lifecycle: no Leonard top-level issue comment existed to edit; kept the still-current stale-collateral inline finding; updated the earlier Leonard back-navigation inline wording to remove stale smoke-specific phrasing; added fresh inline comments for the current head.
…ration # Conflicts: # components/entities/operation/OperationReviewModal.vue # package-lock.json # package.json # pages/position/[number]/borrow/swap.vue # pages/position/[number]/collateral/swap.vue # pages/position/[number]/index.vue # utils/sdk-snapshot-populate-stubs.ts
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pages/position/[number]/index.vue (2)
725-729:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
collateralVaultbefore seedingcollateralItems.When
positionexists butcollateralVault.valueis unavailable, this inserts an undefined vault intocollateralItems; downstream row code dereferencesitem.vault.addressbeforeloadCollaterals()necessarily replaces it.Proposed fix
position.value = getPositionBySubAccountIndex(+positionIndex) if (position.value) { - collateralItems.value = [{ - vault: collateralVault.value as EVault, - assets: position.value.supplied, - }] + const initialCollateralVault = collateralVault.value + collateralItems.value = initialCollateralVault + ? [{ + vault: initialCollateralVault, + assets: position.value.supplied, + }] + : [] // Load collaterals: always for multi-collateral, or when oracle failed (to get actual assets) if (positionCollateralAddresses.value.length > 1 || hasQueryFailure.value) {🤖 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 `@pages/position/`[number]/index.vue around lines 725 - 729, The code seeds collateralItems with a collateralVault that may be undefined, causing downstream code that dereferences item.vault.address to fail. Add a guard condition to check that both position.value and collateralVault.value are available before populating the collateralItems array; only set collateralItems when both conditions are met to prevent undefined vault objects from being inserted into the array.
600-618:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not turn account-lens failures into zero-balance collateral rows.
If
getAccountInfofails for a non-primary collateral,assetsremains0nand the row is still rendered as if the user has no balance. Fall back to the SDK collateral entry when available, or skip the row and surface the query failure instead of showing a synthetic zero.Proposed fix
- let assets = 0n + let assets: bigint | undefined try { const res = await client.readContract({ address: lensAddress as Address, abi: eulerAccountLensABI as Abi, @@ authorizationList: undefined, }) as Record<string, Record<string, unknown>> assets = res.vaultAccountInfo.assets as bigint } catch { if (address === primaryAddress) { assets = position.value!.supplied } + else { + const matchedCollateral = position.value!.collaterals?.find(collateral => { + try { + return getAddress(collateral.vaultAddress || collateral.vault?.address || '') === address + } + catch { + return false + } + }) + assets = matchedCollateral?.assets + } } - return vault ? { vault, assets } : null + return vault && assets !== undefined ? { vault, assets } : null🤖 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 `@pages/position/`[number]/index.vue around lines 600 - 618, The catch block in the getAccountInfo call only handles the primary address case, leaving assets as 0n for non-primary addresses when the query fails, which renders a misleading zero-balance row. Modify the catch block to also handle the non-primary address case by either returning null to skip rendering the row entirely, or falling back to an alternative data source (such as the SDK collateral entry) instead of allowing assets to remain 0n. This ensures failed queries are not shown as zero balances to the user.
🤖 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 `@composables/repay/useCollateralSwapRepay.ts`:
- Line 326: The condition on Line 326 in the useCollateralSwapRepay composable
has inverted logic using AND when it should use OR. The current condition `if
(!isConnected.value && !isSpyMode.value) return false` only disables submit when
BOTH the user is disconnected AND not in spy mode, but it should disable submit
for any disconnected user regardless of spy mode. Change the AND operator to OR
so that the function returns false to disable submit whenever the user is not
connected, unless they are in spy mode which may have special permissions.
In `@composables/repay/useWalletRepay.ts`:
- Line 131: The condition at line 131 in the useWalletRepay composable has
inverted logic: when the wallet is not connected and not in spy mode
(!isConnected.value && !isSpyMode.value), the function currently returns false
which enables the submit action when it should be blocked. Change the return
value from false to true in this condition to properly disable the submit action
for disconnected non-spy users.
In `@composables/useWallets.ts`:
- Around line 165-166: In the useWallets composable, the condition at line 166
that checks if balanceAddress.value matches targetAddress applies getAddress()
normalization only to balanceAddress.value but not to targetAddress, causing the
comparison to fail when addresses differ only in case. Fix this by normalizing
both sides of the comparison: apply getAddress() to targetAddress as well, so
the condition compares getAddress(balanceAddress.value) ===
getAddress(targetAddress) to ensure consistent address normalization on both
sides of the equality check.
---
Outside diff comments:
In `@pages/position/`[number]/index.vue:
- Around line 725-729: The code seeds collateralItems with a collateralVault
that may be undefined, causing downstream code that dereferences
item.vault.address to fail. Add a guard condition to check that both
position.value and collateralVault.value are available before populating the
collateralItems array; only set collateralItems when both conditions are met to
prevent undefined vault objects from being inserted into the array.
- Around line 600-618: The catch block in the getAccountInfo call only handles
the primary address case, leaving assets as 0n for non-primary addresses when
the query fails, which renders a misleading zero-balance row. Modify the catch
block to also handle the non-primary address case by either returning null to
skip rendering the row entirely, or falling back to an alternative data source
(such as the SDK collateral entry) instead of allowing assets to remain 0n. This
ensures failed queries are not shown as zero balances to the user.
🪄 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: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58904d9b-fbbb-42fa-85e4-9c7fd0c51594
📒 Files selected for processing (12)
components/entities/operation/OperationReviewModal.vuecomposables/borrow/useMultiplyCowSwap.tscomposables/repay/useCollateralSwapRepay.tscomposables/repay/useRepaySwapCore.tscomposables/repay/useWalletRepay.tscomposables/repay/useWalletSwapRepay.tscomposables/useEulerTx.tscomposables/useWallets.tspages/position/[number]/borrow/swap.vuepages/position/[number]/index.vuescripts/execution-record.mjsutils/stepDecoding.ts
✅ Files skipped from review due to trivial changes (3)
- composables/repay/useRepaySwapCore.ts
- scripts/execution-record.mjs
- composables/repay/useWalletSwapRepay.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- components/entities/operation/OperationReviewModal.vue
- composables/useEulerTx.ts
- utils/stepDecoding.ts
- pages/position/[number]/borrow/swap.vue
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pages/position/[number]/index.vue (2)
725-729:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
collateralVaultbefore seedingcollateralItems.When
positionexists butcollateralVault.valueis unavailable, this inserts an undefined vault intocollateralItems; downstream row code dereferencesitem.vault.addressbeforeloadCollaterals()necessarily replaces it.Proposed fix
position.value = getPositionBySubAccountIndex(+positionIndex) if (position.value) { - collateralItems.value = [{ - vault: collateralVault.value as EVault, - assets: position.value.supplied, - }] + const initialCollateralVault = collateralVault.value + collateralItems.value = initialCollateralVault + ? [{ + vault: initialCollateralVault, + assets: position.value.supplied, + }] + : [] // Load collaterals: always for multi-collateral, or when oracle failed (to get actual assets) if (positionCollateralAddresses.value.length > 1 || hasQueryFailure.value) {🤖 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 `@pages/position/`[number]/index.vue around lines 725 - 729, The code seeds collateralItems with a collateralVault that may be undefined, causing downstream code that dereferences item.vault.address to fail. Add a guard condition to check that both position.value and collateralVault.value are available before populating the collateralItems array; only set collateralItems when both conditions are met to prevent undefined vault objects from being inserted into the array.
600-618:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not turn account-lens failures into zero-balance collateral rows.
If
getAccountInfofails for a non-primary collateral,assetsremains0nand the row is still rendered as if the user has no balance. Fall back to the SDK collateral entry when available, or skip the row and surface the query failure instead of showing a synthetic zero.Proposed fix
- let assets = 0n + let assets: bigint | undefined try { const res = await client.readContract({ address: lensAddress as Address, abi: eulerAccountLensABI as Abi, @@ authorizationList: undefined, }) as Record<string, Record<string, unknown>> assets = res.vaultAccountInfo.assets as bigint } catch { if (address === primaryAddress) { assets = position.value!.supplied } + else { + const matchedCollateral = position.value!.collaterals?.find(collateral => { + try { + return getAddress(collateral.vaultAddress || collateral.vault?.address || '') === address + } + catch { + return false + } + }) + assets = matchedCollateral?.assets + } } - return vault ? { vault, assets } : null + return vault && assets !== undefined ? { vault, assets } : null🤖 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 `@pages/position/`[number]/index.vue around lines 600 - 618, The catch block in the getAccountInfo call only handles the primary address case, leaving assets as 0n for non-primary addresses when the query fails, which renders a misleading zero-balance row. Modify the catch block to also handle the non-primary address case by either returning null to skip rendering the row entirely, or falling back to an alternative data source (such as the SDK collateral entry) instead of allowing assets to remain 0n. This ensures failed queries are not shown as zero balances to the user.
🤖 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 `@composables/repay/useCollateralSwapRepay.ts`:
- Line 326: The condition on Line 326 in the useCollateralSwapRepay composable
has inverted logic using AND when it should use OR. The current condition `if
(!isConnected.value && !isSpyMode.value) return false` only disables submit when
BOTH the user is disconnected AND not in spy mode, but it should disable submit
for any disconnected user regardless of spy mode. Change the AND operator to OR
so that the function returns false to disable submit whenever the user is not
connected, unless they are in spy mode which may have special permissions.
In `@composables/repay/useWalletRepay.ts`:
- Line 131: The condition at line 131 in the useWalletRepay composable has
inverted logic: when the wallet is not connected and not in spy mode
(!isConnected.value && !isSpyMode.value), the function currently returns false
which enables the submit action when it should be blocked. Change the return
value from false to true in this condition to properly disable the submit action
for disconnected non-spy users.
In `@composables/useWallets.ts`:
- Around line 165-166: In the useWallets composable, the condition at line 166
that checks if balanceAddress.value matches targetAddress applies getAddress()
normalization only to balanceAddress.value but not to targetAddress, causing the
comparison to fail when addresses differ only in case. Fix this by normalizing
both sides of the comparison: apply getAddress() to targetAddress as well, so
the condition compares getAddress(balanceAddress.value) ===
getAddress(targetAddress) to ensure consistent address normalization on both
sides of the equality check.
---
Outside diff comments:
In `@pages/position/`[number]/index.vue:
- Around line 725-729: The code seeds collateralItems with a collateralVault
that may be undefined, causing downstream code that dereferences
item.vault.address to fail. Add a guard condition to check that both
position.value and collateralVault.value are available before populating the
collateralItems array; only set collateralItems when both conditions are met to
prevent undefined vault objects from being inserted into the array.
- Around line 600-618: The catch block in the getAccountInfo call only handles
the primary address case, leaving assets as 0n for non-primary addresses when
the query fails, which renders a misleading zero-balance row. Modify the catch
block to also handle the non-primary address case by either returning null to
skip rendering the row entirely, or falling back to an alternative data source
(such as the SDK collateral entry) instead of allowing assets to remain 0n. This
ensures failed queries are not shown as zero balances to the user.
🪄 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: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58904d9b-fbbb-42fa-85e4-9c7fd0c51594
📒 Files selected for processing (12)
components/entities/operation/OperationReviewModal.vuecomposables/borrow/useMultiplyCowSwap.tscomposables/repay/useCollateralSwapRepay.tscomposables/repay/useRepaySwapCore.tscomposables/repay/useWalletRepay.tscomposables/repay/useWalletSwapRepay.tscomposables/useEulerTx.tscomposables/useWallets.tspages/position/[number]/borrow/swap.vuepages/position/[number]/index.vuescripts/execution-record.mjsutils/stepDecoding.ts
✅ Files skipped from review due to trivial changes (3)
- composables/repay/useRepaySwapCore.ts
- scripts/execution-record.mjs
- composables/repay/useWalletSwapRepay.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- components/entities/operation/OperationReviewModal.vue
- composables/useEulerTx.ts
- utils/stepDecoding.ts
- pages/position/[number]/borrow/swap.vue
🛑 Comments failed to post (3)
composables/repay/useCollateralSwapRepay.ts (1)
326-326:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisconnected non-spy submit gate is inverted.
At Line 326,
if (!isConnected.value && !isSpyMode.value) return falseenables submit for a disconnected non-spy user. This should disable submit to prevent invalid execution attempts.Suggested fix
- if (!isConnected.value && !isSpyMode.value) return false + if (!isConnected.value && !isSpyMode.value) return true🤖 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 `@composables/repay/useCollateralSwapRepay.ts` at line 326, The condition on Line 326 in the useCollateralSwapRepay composable has inverted logic using AND when it should use OR. The current condition `if (!isConnected.value && !isSpyMode.value) return false` only disables submit when BOTH the user is disconnected AND not in spy mode, but it should disable submit for any disconnected user regardless of spy mode. Change the AND operator to OR so that the function returns false to disable submit whenever the user is not connected, unless they are in spy mode which may have special permissions.composables/repay/useWalletRepay.ts (1)
131-131:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubmit-disabled guard currently enables disconnected non-spy flow.
At Line 131, returning
falsefor!isConnected && !isSpyModemakes the action appear submittable when it should be blocked.Suggested fix
- if (!isConnected.value && !isSpyMode.value) return false + if (!isConnected.value && !isSpyMode.value) return true📝 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.if (!isConnected.value && !isSpyMode.value) return true🤖 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 `@composables/repay/useWalletRepay.ts` at line 131, The condition at line 131 in the useWalletRepay composable has inverted logic: when the wallet is not connected and not in spy mode (!isConnected.value && !isSpyMode.value), the function currently returns false which enables the submit action when it should be blocked. Change the return value from false to true in this condition to properly disable the submit action for disconnected non-spy users.composables/useWallets.ts (1)
165-166:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAddress comparison can incorrectly discard valid wallet fetch results.
At Line 166, the condition compares
getAddress(balanceAddress.value)(normalized) totargetAddress(possibly not normalized). Case-only differences can make this false and prevent balances from updating.Suggested fix
- const targetAddress = balanceAddress.value as Address + const targetAddress = getAddress(balanceAddress.value as Address) ... - if (chainId.value === currentChainId && balanceAddress.value && getAddress(balanceAddress.value) === targetAddress) { + if (chainId.value === currentChainId && balanceAddress.value && getAddress(balanceAddress.value) === targetAddress) {🤖 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 `@composables/useWallets.ts` around lines 165 - 166, In the useWallets composable, the condition at line 166 that checks if balanceAddress.value matches targetAddress applies getAddress() normalization only to balanceAddress.value but not to targetAddress, causing the comparison to fail when addresses differ only in case. Fix this by normalizing both sides of the comparison: apply getAddress() to targetAddress as well, so the condition compares getAddress(balanceAddress.value) === getAddress(targetAddress) to ensure consistent address normalization on both sides of the equality check.
LeonardEulerXYZ
suggested changes
Jun 22, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 647654a.
Verdict: request changes. The refinance consolidation is directionally good and the current preview renders the combined debt/collateral flow on both desktop and mobile, but the current head still has several route-state issues that can make the action operate on the wrong context or lose the user’s intended network. I am keeping the existing Leonard inline comments as the concrete coordinates rather than reposting duplicates.
Blocking findings still relevant on this head:
pages/position/[number]/borrow/swap.vue:route.query.collateralcan still fall back togetVault(queryCollateral)even when that collateral is not one of the loaded position collaterals. A crafted/stale URL can therefore setsourceCollateralVaultto a known but unrelated vault while the position collateral lookup is null.pages/position/[number]/borrow/swap.vue: debt/collateral modal selection still passes a numeric row index back into parallel vault arrays. With async option generation and context changes, this can select a vault different from the row the user clicked. Selection should resolve by option/vault address, not by a stale display index.pages/position/[number]/borrow/swap.vueandpages/position/[number]/index.vue: fallback/back/refinance links still drop active query state such asnetwork(and in some cases the selectedcollateral), which can move non-default-network users into the wrong route context.
Validation performed:
- Focused tests:
npm run test:run -- tests/golden/tx-plan-builders.test.ts tests/utils/stepDecoding.test.tspassed. - Typecheck:
npm run typecheckpassed. - Production build:
npm run buildpassed. - Browser smoke: preview route
/position/1/borrow/swap?network=1&spy=<public-mainnet-fixture>rendered the Refinance flow with both Collateral vault and Debt vault sections on desktop and mobile. I did not perform wallet signing. - Active bot feedback: I checked the current CodeRabbit comments against the diff; the material suggestions around
allBorrowOptions, test fixture target contracts, and withdraw asset decoding are addressed/non-blocking on this head. The remaining blockers above are from direct code review.
Smoke coverage: browser visual smoke + mobile smoke; no wallet/signing smoke.
Screenshots from the preview smoke:
Desktop, 1365x900 — combined refinance route loaded with collateral and debt vault controls:
Mobile, 390x844 — same route at narrow viewport:
- add refinance entries to the batch builder - keep simulated position highlights and removed rows scoped to changed assets - decode batch execution errors and separate quoted amounts from verifier minimums
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@composables/useTxBatch.ts`:
- Around line 265-292: The buildRefinanceReplacementBorrowPositionKeys function
constructs position keys using sourceCollateralVaults from the review metadata,
which only includes collateral vaults with positive assets due to filtering in
currentCollateralLegs. However, the existing position key includes all enabled
collateral vaults regardless of asset balance, causing a mismatch. Ensure that
sourceCollateralVaults includes all enabled collateral vaults with zero assets
in addition to those with positive assets, so the keys generated for refinance
replacement positions match the structure of existing borrow position keys.
🪄 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: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2d0504d-ef68-43ad-b2d5-619ae2f53bfb
📒 Files selected for processing (6)
composables/useTxBatch.tspages/position/[number]/borrow/swap.vuepages/position/[number]/index.vuetests/composables/useTxBatchModifiedKeys.test.tstests/utils/stepDecoding.test.tsutils/stepDecoding.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/utils/stepDecoding.test.ts
- pages/position/[number]/borrow/swap.vue
- utils/stepDecoding.ts
- pages/position/[number]/index.vue
LeonardEulerXYZ
suggested changes
Jun 23, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head c9a4579.
Verdict: request changes.
The combined refinance route is much closer than the earlier heads, and the lightweight validation path is mostly healthy, but I still see merge-blocking issues around URL-controlled collateral source selection and a new lint failure. I also verified one current CodeRabbit concern as materially relevant, with a narrower explanation below.
Blocking findings:
- The prior Leonard comment on the crafted/stale
collateral=query still applies.sourceCollateralVaultstill falls back to the global vault registry when the query address is not one of the position’s collateral legs. That value then feedsinput.collateral.fromVaultwithcurrentCollateralAssets, which falls back to the position’s primary supplied amount when no matching source collateral position exists. A bookmarked/crafted URL can therefore make the planner/UI reason about the wrong source collateral vault. This should be constrained to the position’s actual collateral set, or the query should be ignored/cleared when it does not match. - Lint now fails on
composables/useSwapCollateralOptions.ts:56(no-useless-assignment):let candidates: EVault[] = []is overwritten by both branches before it is read.
Warnings / follow-ups:
- The prior Leonard network-preservation comments still apply: the collateral-row Refinance link and the refinance back fallback still drop the active
networkquery. This is not as dangerous as the source-collateral issue, but it can move users out of the network/context they were reviewing. - The prior Leonard numeric row-index selection comment still appears applicable: the modal event still passes a displayed row index and then indexes
debtSelectionVaults/collateralSelectionVaults. Resolving by selected vault address would be safer when async option maps and synchronous vault arrays update around a context change. - CodeRabbit’s latest
useTxBatchconcern is substantively valid, but the sharper edge is this:sourceCollateralVaultsis built from positive-balancecurrentCollateralLegs, while the canonical borrow-position key includesposition.collateralVaultsplus collateral/current-collateral addresses. If a position has a zero-balance enabled collateral, the replacement suppression key can fail to match and the batch overlay can still show the old borrow position as removed. See inline.
Validation performed:
- Inspected PR metadata, current head, changed-file list, diff, surrounding refinance/portfolio code, prior Leonard reviews/comments, and active CodeRabbit review comments.
- Focused tests passed:
npm run test:run -- tests/composables/useTxBatchModifiedKeys.test.ts tests/utils/stepDecoding.test.ts tests/composables/useRepaySwapCore.test.ts tests/composables/useWalletSwapRepay.test.ts tests/utils/swap-route-visibility.test.ts→ 25 tests passed. npm run typecheckpassed.npm run buildpassed.- Targeted lint failed:
npx eslint composables/useSwapCollateralOptions.ts pages/position/[number]/borrow/swap.vue pages/position/[number]/index.vue composables/useTxBatch.ts→ oneno-useless-assignmenterror. - Browser visual smoke: local production build, headed Chromium under Xvfb, desktop 1366×900 and mobile 390×844 on
/position/0/borrow/swap?network=1; both rendered the refinance shell without page errors. This was disconnected route/mobile smoke only, not wallet/signing or transaction execution coverage.
Screenshots from the disconnected route smoke:
Scalability / maintainability hygiene pass:
- The PR consolidates collateral swap into the refinance route instead of leaving sibling route logic split; that direction is good.
- The remaining risky pieces are exactly the shared state boundaries: route query parsing, modal selection identity, and batch overlay position-key construction. These should use canonical position/vault identity rather than local fallbacks or display-array indexes, because the same concepts are reused across debt-only, collateral-only, and combined refinance flows.
Smoke coverage: browser visual smoke + mobile smoke on disconnected route shell; no wallet/signing smoke.
Comment lifecycle: no Leonard top-level issue comment existed to edit; I kept the existing Leonard inline comments that remain relevant and did not delete other users’/bots’ comments.
- constrain collateral query selection to position collaterals - preserve network query in refinance navigation - resolve modal selections by selected vault address - include all source collateral vaults in refinance replacement metadata - fix disconnected submit guards and wallet address normalization
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 23, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head fa63ca1.
Verdict: request changes — the previous Leonard route-state blockers are fixed on this head, but one sibling entry point still drops the active network query before entering the consolidated refinance flow.
Scope reviewed:
- consolidated refinance route state for debt/collateral/both migrations
- position-page refinance entry points
- modal selection identity and stale-query handling
- batch replacement key / step-decoding changes
- CoW collateral-only refinance gating
- active CodeRabbit comments, verified against current code
Finding:
- Blocking: the top-level position “Refinance” button still links to
/position/${positionIndex}/borrow/swapwithout preservingroute.query.network. The collateral-row refinance link now usesbuildRefinanceRoute(...)and keepsnetwork, so this is the remaining sibling inconsistency. On a non-default-network position page, this entry point can move the user into the default/wrong route context before they review the refinance.
Validation performed:
- Focused tests passed:
npm run test:run -- tests/utils/stepDecoding.test.ts tests/composables/useTxBatchModifiedKeys.test.ts tests/golden/tx-plan-builders.test.ts(Vitest matched and ran the two relevant suites, 19 tests passed). npm run typecheckpassed.npm run buildpassed.- Focused lint over the touched refinance/batch/decoding files passed.
- Full
npm run lintstill fails on existing unrelated files outside this PR’s changed surface; I did not treat that as a PR blocker. - Browser visual smoke on the Railway PR preview passed for the disconnected refinance shell on desktop and mobile:
/position/0/borrow/swap?network=1returned 200 and rendered the refinance page. I also checked/position/0?network=1; disconnected mode does not expose the actionable top-level position button, so the network-preservation issue above is code-path verified rather than browser-click verified.
Screenshots from the PR preview smoke:
Bot/reviewer feedback pass:
- CodeRabbit’s current
useTxBatchsource-collateral-vaults concern is stale/false positive on this head: the PR now buildssourceCollateralVaultAddressesfrom the full current position collateral set, and the focused batch modified-key test covers the multi-collateral replacement case. - CodeRabbit’s
stepDecodingwithdraw asset concern is also stale/false positive on this head: Withdraw now resolves the asset frombatchItem.targetContract, and the test covers a receiver that is itself another known vault. - CodeRabbit’s multicall fixture concern is resolved in the current test fixture.
Scalability / maintainability hygiene pass:
- Good: the PR consolidates the old standalone collateral-swap route into the shared refinance flow and moves more of the shared transaction review semantics into the common batch/step-decoding path.
- Remaining issue: route construction is still duplicated between the top-level position action and the collateral-row action. The collateral-row helper has the correct network-preserving behavior; the top-level sibling should use the same helper or a shared query-preserving route builder so future refinance entry points cannot drift again.
Smoke coverage: browser visual smoke + mobile smoke on disconnected route shell; no wallet/signing smoke or transaction execution coverage.
Comment lifecycle: deleted four stale Leonard inline comments from the previous head that are fixed on this head; submitted this fresh review against fa63ca1.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 24, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 8f80162.
Verdict: request changes. The latest commit fixes the prior Leonard network-preservation blocker, and the route/build/test smoke is healthy, but I found one remaining transaction-risk issue in the new combined refinance path.
Scope reviewed:
- consolidated refinance route:
pages/position/[number]/borrow/swap.vue - position entry links and query-state preservation in
pages/position/[number]/index.vue - transaction planning/batch metadata in
composables/useEulerTx.tsandcomposables/useTxBatch.ts - swap option helpers and step-decoding changes/tests
- active CodeRabbit/reviewer comments, verified against current head rather than taken as authority
Finding:
- Blocking: debt-swap price impact is signed in the wrong direction before passing into the shared high-price-impact gate. Bad debt quotes that require more target debt to repay the same old debt become positive impacts and can bypass the
<= -10%danger confirmation. See inline comment.
Validation performed:
npm run typecheck— passednpm run test:run -- tests/composables/useTxBatchModifiedKeys.test.ts tests/utils/stepDecoding.test.ts tests/composables/useSwapPageLogic.test.ts— passed, 22 testsnpm run build— passed, warnings only- headed Chromium/Xvfb browser smoke on Railway preview:
- desktop
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page-error text - mobile
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page-error text - desktop
/position/0?network=1returned 200 and rendered portfolio shell; disconnected mode does not expose position action buttons
- desktop
- Wallet/signing smoke: not run.
Scalability / maintainability hygiene pass:
- The prior route-link drift is fixed on current head: both top-level and collateral-row Refinance entries use the same
buildRefinanceRoute(...)helper and preservenetwork. - No stale
/position/:number/collateral/swapreferences found. - The unified route is necessarily larger, but it now concentrates the debt/collateral refinance action in one place. Follow-up worth considering, not blocking this review: the PR deletes the old collateral-swap execution scenario and the remaining execution scenario still appears named around the old swap-debt UI; add/update an execution scenario for selecting a collateral target on the unified refinance route so the main collateral-migration intent is covered end-to-end.
Bot/reviewer feedback pass:
- Current CodeRabbit inline threads I checked are stale/resolved or already addressed on this head:
allBorrowOptionsdependency, explicit swapper test fixture target, Withdraw asset decoding, and refinance metadata coverage for zero-balance enabled collaterals. - I did not promote any bot claim to a Leonard finding without re-checking the code/tests.
Comment lifecycle:
- Deleted one stale Leonard-owned inline comment from the previous head about the top-level Refinance link dropping
network; current code fixes it. - No prior Leonard top-level walkthrough comment existed to edit, so this is a fresh formal review for the current head.
Screenshots:
- Captured local desktop/mobile smoke screenshots, but did not attach them to GitHub:
gh gist createrejected PNG binaries, and the blocking finding is code-path based rather than visual. The browser smoke evidence is summarized above.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head ad542b5.
Verdict: comment — no remaining blockers found in this incremental re-review. The latest commit fixes the previous Leonard blocker: debt-swap price impact now treats worse debt quotes as negative impact before feeding the shared price-impact gate.
Scope reviewed:
- Incremental diff from the last Leonard-reviewed head:
pages/position/[number]/borrow/swap.vue - Prior route-state blocker in
pages/position/[number]/index.vue - Prior batch/step-decoding/metadata concerns around the consolidated refinance flow
- Active CodeRabbit/reviewer comments checked against current code rather than accepted as authority
Validation performed:
npm run typecheck— passednpm run test:run -- tests/composables/useTxBatchModifiedKeys.test.ts tests/utils/stepDecoding.test.ts tests/composables/useSwapPageLogic.test.ts tests/golden/tx-plan-builders.test.ts— Vitest ran the non-golden matching suites; passed, 22 testsnpm run test:run -- tests/golden/tx-plan-builders.test.ts— not runnable through the normal Vitest script becausetests/golden/**is excluded by confignpm run build— passed, warnings only- Headed Chromium/Xvfb browser smoke on the Railway PR preview:
- desktop
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page errors - mobile
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page errors; one COOP fetch console error was observed and treated as browser/preview noise, not a route failure
- desktop
Scalability / maintainability hygiene pass:
- The prior duplicated route-construction drift is fixed: both top-level and collateral-row Refinance entries now go through
buildRefinanceRoute(...)and preservenetwork. - The consolidated refinance route remains large, but the PR is at least concentrating debt/collateral migration behavior in one flow rather than adding another one-off sibling path.
- Non-blocking follow-up remains useful: restore/add an execution scenario that exercises selecting a collateral target on the unified refinance route, since the old standalone collateral-swap scenario was removed and this PR’s main user-visible intent is collateral migration.
Bot/reviewer feedback pass:
- Current CodeRabbit comments I checked are stale/resolved or non-blocking on this head:
allBorrowOptionsnow includescollateralVaultas a reactive dependency; withdraw step decoding usestargetContract; the multicall fixture target concern is covered in the current tests; the refinance metadata concern is covered by the current full-collateral-set replacement metadata/tests.
Smoke coverage: browser route smoke + mobile smoke on the disconnected refinance shell; no wallet/signing smoke or live quote/transaction execution coverage.
Screenshots: not posted for this incremental review. I did not capture new screenshots because the current head only changes non-visual price-impact arithmetic, and the route smoke was textual/health-oriented rather than visual evidence.
Comment lifecycle: deleted the stale Leonard-owned inline comment about debt price-impact sign after verifying the current head fixes it; no Leonard top-level walkthrough comment existed to edit.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head a088f40.
Verdict: comment — no remaining blockers found in this incremental re-review. The latest commit only changes the refinance summary display gate, and the new showNextRefinanceMetrics condition is consistent with the route validation/CTA gates: no “after” metrics are shown until the user has selected a debt/collateral change and all required quotes are present.
Scope reviewed:
- Incremental diff from the last Leonard-reviewed head:
pages/position/[number]/borrow/swap.vue - Consolidated refinance route metric readiness, validation errors, and CTA gating
- Position-page refinance entry links and stale
/collateral/swapreferences - Active CodeRabbit/reviewer comments checked against current code rather than treated as authority
Validation performed:
npm run typecheck— passednpm run test:run -- tests/composables/useTxBatchModifiedKeys.test.ts tests/utils/stepDecoding.test.ts tests/composables/useSwapPageLogic.test.ts— passed, 22 testsnpx eslint pages/position/[number]/borrow/swap.vue pages/position/[number]/index.vue composables/useSwapCollateralOptions.ts composables/useSwapDebtOptions.ts composables/useTxBatch.ts utils/stepDecoding.ts— passednpm run build— passed, warnings only- Headed Chromium/Xvfb route smoke on the PR preview:
- desktop
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page errors - mobile
/position/0/borrow/swap?network=1returned 200 and rendered the Refinance shell with no page errors
- desktop
Scalability / maintainability hygiene pass:
- No stale
/position/:number/collateral/swaproute references found after the route consolidation. - The prior route-construction drift remains fixed: the top-level debt Refinance action and collateral-row Refinance action both use the same
buildRefinanceRoute(...)helper and preservenetwork. - The new metric display gate is localized, but it matches analogous readiness gating patterns in sibling action routes. I did not find a sibling surface still showing the old “after metrics while no refinance change exists” behavior.
- Non-blocking coverage gap remains worth addressing: the old collateral-swap execution scenario was removed, and I do not see a replacement execution scenario that opens the collateral-row Refinance action and selects a collateral target through the unified route. The current focused tests cover batch/decoding logic, but not the end-to-end collateral-migration UI path.
Bot/reviewer feedback pass:
- The current CodeRabbit comments I checked are stale/resolved or non-blocking on this head: borrow-option dependency coverage, withdraw asset decoding, multicall fixture target, and refinance batch metadata coverage are covered by the current code/tests.
- I did not promote any bot claim to a Leonard finding without re-checking the code.
Smoke coverage: browser route smoke + mobile smoke on the disconnected refinance shell; no wallet/signing smoke, live quote selection, or transaction execution coverage.
Screenshots: not posted for this incremental review. I did not capture new screenshots because the current head changes metric display gating and the smoke was a textual route-health check, not a visual regression artifact.
Comment lifecycle: no Leonard top-level walkthrough comment exists to edit; no Leonard-owned inline review comments remain on the current head to delete or update.
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 26, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 1de90ff8b3bb62e2b566854aaa79048dbdcf0f71.
Verdict: request changes. The consolidated refinance direction is much cleaner, and the current test/build path is healthy, but I found two route-state regressions that should be fixed before merge.
Scope reviewed:
- consolidated refinance route and position-page entry links
- collateral/debt target selection and query preselection
- removed standalone collateral swap route behavior
- batch/review metadata and step decoding changes
- current CodeRabbit/bot comments as evidence, not authority
- scalability / maintainability hygiene for sibling refinance flows and option-selection helpers
Validation performed:
npm run test:run -- tests/composables/useTxBatchModifiedKeys.test.ts tests/utils/stepDecoding.test.ts tests/composables/useSwapPageLogic.test.ts— passed, 24 testsnpx eslint pages/position/[number]/borrow/swap.vue pages/position/[number]/index.vue composables/useSwapCollateralOptions.ts composables/useSwapDebtOptions.ts composables/useTxBatch.ts utils/stepDecoding.ts— passednpm run typecheck— passednpm run build— passed, with existing bundle/chunk warnings only- headed Chromium / Xvfb preview smoke on
https://dev-build-euler-lite-pr-575.up.railway.app:- desktop + mobile
/position/1/borrow/swap?network=1&spy=...rendered the Refinance route shell without a Nuxt error, but the selected public fixture did not hydrate far enough to show the vault selectors before timeout, so this is incomplete route smoke rather than full interaction coverage. - desktop
/position/1/collateral/swap?network=1&spy=...rendered a page-not-found error.
- desktop + mobile
Blocking findings:
- The old collateral swap route now 404s instead of redirecting into the consolidated refinance flow. This breaks existing links/bookmarks and contradicts the PR body’s “remove/redirect” claim.
- A stale
?to=debt target can be re-applied after the user explicitly chooses to keep the current debt vault, if collateral changes later recompute the debt target list.
Non-blocking gaps worth addressing soon:
- Execution scenario coverage is stale/incomplete for the new unified refinance UI: the old collateral-swap scenario was removed, and the remaining swap-debt scenario still appears to target old labels like
To/Review Swaprather than the newDebt vault/Review Refinanceflow. - Scalability / maintainability hygiene: the new grouped option-selection behavior is subtle and reusable, but mostly lives in the modal/page path. Focused tests around grouped option index preservation, address-based selection, and the new
useSwapCollateralOptions/useSwapDebtOptionscandidate filters would make this less fragile.
Bot/reviewer comment pass:
- I verified the older CodeRabbit comments against the current head. The
allBorrowOptionsdependency, withdraw asset decoding, explicit multicall target fixture, selector-argument matching, and full-collateral batch metadata concerns appear addressed or no longer materially actionable on this head. I did not promote any unverified bot claim to a Leonard finding.
Screenshot evidence:
- Old collateral route 404:
- Desktop refinance route shell:
- Mobile refinance route shell:
Smoke coverage: incomplete browser visual smoke; no wallet/signing coverage.
…ration # Conflicts: # components/BatchReviewModal.vue # components/entities/operation/OperationReviewModal.vue # pages/position/[number]/borrow/swap.vue # pages/position/[number]/collateral/swap.vue
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-575
LeonardEulerXYZ
suggested changes
Jun 26, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head a304568f18eded87b8d317ccfd6793964d642c03.
Verdict: request changes. The current head still has two route-state blockers in the consolidated refinance flow. I kept the existing Leonard inline comments on this same head as the coordinates rather than reposting duplicate inline comments.
Blocking findings:
pages/position/[number]/collateral/swap.vuedeletion still leaves the old collateral-swap URL as a hard 404. The PR states standalone swap-collateral entry points are routed into refinance, but the deployed PR preview rendersPage not foundfor/position/1/collateral/swap?network=1on both desktop and mobile. Existing bookmarks, external links, or stale client navigation should redirect/stub into the consolidated refinance route while preserving relevant query state.pages/position/[number]/borrow/swap.vuestill treatsroute.query.toas a persistent preselection source. If a user lands with?to=<debtVault>, manually chooses the current debt vault to clear the debt change, and then changes collateral, the watcher can run again afterdebtTargetVaultsrecomputes and silently reselect the stale debt target. That can turn an intended collateral-only refinance into mixed debt+collateral refinance.
Validation performed:
- Inspected PR metadata, current head, changed files, full PR diff, surrounding route/planner/review code, prior Leonard reviews/comments, and current CodeRabbit inline comments.
npm run test:run— passed, 98 files / 994 tests.- Focused route/planner tests:
tests/composables/useTxBatchModifiedKeys.test.tsandtests/utils/stepDecoding.test.tspassed;tests/golden/tx-plan-builders.test.tsis excluded by the repo Vitest config, so it did not run throughtest:run. npm run typecheck— passed.npm run build— passed.npm run lint— failed, but the remaining errors are also present onorigin/development; not counted as a PR-specific blocker.- Headed/Xvfb Playwright smoke against the Railway PR preview:
- desktop and mobile
/position/1/collateral/swap?network=1both renderedError: Page not found. - desktop and mobile
/position/1/borrow/swap?network=1rendered the consolidatedRefinancepage shell.
- desktop and mobile
Scalability / maintainability hygiene pass:
- Checked sibling action entry points and the consolidated refinance source/target query model, not only the touched watcher. The main remaining consistency issue is the old-route compatibility gap: deleting the legacy route solves the visible component path but leaves sibling/stale navigation and bookmarks without a reusable redirect/mapping layer.
- The
topreselection behavior is still local watcher state rather than a one-shot/consumed URL-state abstraction. Because this route now composes collateral and debt migration in one form, stale query state is transaction-relevant and should be normalized at the route boundary. - No new one-off formatter/business-logic duplication stood out in this pass beyond the route-state issues above.
Bot/reviewer feedback pass:
- Verified CodeRabbit's stale
allBorrowOptionsdependency concern is no longer applicable on current code:collateralVaultis included in theuseReactiveMapdependency list. - Verified CodeRabbit's withdraw asset-resolution concern is no longer applicable on current code:
Withdrawnow resolves the source asset fromtargetContract. - I did not promote the remaining test-fixture tightening suggestions to blockers.
Smoke coverage: browser visual/text route smoke on desktop and mobile; no wallet/signing smoke. Screenshots were not posted because the remaining evidence is route-state/textual (Page not found) rather than a subtle visual regression.
LeonardEulerXYZ
approved these changes
Jul 1, 2026
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head e814d9deb73c4820c8446bbe305a80eb53d809f4.
Verdict: approve — the prior Leonard blockers are resolved on this head, and I did not find a new blocker in the current re-review.
Scope reviewed:
- consolidated refinance route in
pages/position/[number]/borrow/swap.vue - legacy collateral-swap compatibility route in
pages/position/[number]/collateral/swap.vue - route query consumption for
to/targetCollateral - batch/review metadata wiring around collateral/debt refinance quotes
- transaction batch modified-key behavior and step decoding tests
- scalability / maintainability hygiene across sibling refinance-style route state and shared transaction paths
- active bot/reviewer feedback checked as evidence, not authority
Validation performed:
npm run test:run -- tests/utils/stepDecoding.test.ts tests/composables/useTxBatchModifiedKeys.test.ts— passed, 21 tests.npm run typecheck— passed.npm run build— passed; only existing build-time warnings observed.- Built Nuxt server route/API checks:
/returned HTTP 200;/api/rpc/1eth_chainIdreturned0x1. - Headed Playwright/Xvfb browser smoke:
- desktop
/position/1/borrow/swap?network=1&spy=<fixture>renders the refinance form with Collateral vault, Debt vault, warning copy, metrics panel, and Choose Vaults CTA. - mobile same route renders the same refinance controls without a blank/error state.
- legacy
/position/1/collateral/swap?...&to=<target>redirects to/position/1/borrow/swap?...&targetCollateral=<target>and renders the refinance page.
- desktop
Screenshot evidence:
- Desktop refinance route:
- Mobile refinance route:
- Legacy collateral route redirect:
Notes:
- The old collateral route now has a compatibility redirect/stub, so the previous hard-404 finding is fixed.
to/targetCollateralpreselection is now consumed and the route query is cleared after applying, so the previous stale-query re-selection finding is fixed.- The blank smoke attempt with an invalid spy/position pairing was discarded; the posted screenshots use a subgraph-backed borrow/collateral fixture and were checked for the intended visible state before posting.
- Browser smoke was disconnected/spy-mode coverage only; no wallet signing was exercised.
- Non-blocking local noise observed: blank AppKit project id causes Reown 403 warnings in local disconnected smoke, and Merkl allowlist 404s appear for unsupported proxy paths. These did not block the refinance page render.
Scalability / maintainability hygiene:
- This head centralizes the refinance action on the consolidated borrow/swap page rather than reintroducing one-off sibling action logic. Query consumption and target-vault matching are scoped to target-list candidates, which is the right reusable boundary for this route.
- I did not find a sibling route that still carries the old stale
tobehavior or the old collateral-swap hard-404 behavior. The remaining debt/collateral summary helpers are local to this combined planner and covered indirectly by the focused transaction/batch tests plus browser smoke.
Bot/reviewer feedback checked:
- Existing CodeRabbit comments were inspected. I did not find a current material blocker to amplify from them on this head; the active Leonard findings that mattered here were the two prior route-state blockers, both now resolved.
Seranged
approved these changes
Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
unhealthy execution.
flow.
Changes
same-asset and swap-backed paths.
explicit copy.
flow.
Summary by CodeRabbit
New Features
Changes
Documentation
Bug Fixes / Quality