Conversation
|
Warning Rate limit exceeded@vignesha22 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughConsolidates Pulse SDK logic: removes Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Buy Component
participant Hook as useIntentSdk
participant IntentSDK as IntentSdk
participant Chain as Blockchain
Component->>Hook: useIntentSdk({ payingTokens })
Hook->>IntentSDK: initialize with payingTokens
Hook-->>Component: { intentSdk, areModulesInstalled, isInstalling, isFetching, installModules }
rect rgb(220,240,255)
Note over Component,Hook: Module installation flow when modules missing
Component->>Hook: installModules()
Hook->>IntentSDK: installPulseModules(addresses) / sendTransactions()
IntentSDK->>Chain: submit txns
Chain-->>IntentSDK: receipts
Hook-->>Component: installation complete / flags updated
end
rect rgb(240,220,255)
Note over Component,IntentSDK: Intent submission flow
Component->>IntentSDK: intentSdk.expressIntent(params)
IntentSDK->>Chain: submit intent
Chain-->>IntentSDK: result
IntentSDK-->>Component: { intentHash, bids }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to inspect closely:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
Deploying x with
|
| Latest commit: |
744082a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://25fcaa2e.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3824-update-intentsdk.x-e62.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (3)
371-382: Same timers and mock-shape suggestion applies here.Adopt fake timers and return a consistent useIntentSdk mock shape.
411-423: Same timers and mock-shape suggestion applies here.Use vi.useFakeTimers and advanceTimers; keep the hook mock shape consistent.
462-473: Same timers and mock-shape suggestion applies here.Adopt fake timers; mock hook with full return surface.
🧹 Nitpick comments (5)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (3)
27-29: Remove dead commented mock.Delete the commented useModularSdk mock to avoid confusion.
-// vi.mock('../../../hooks/useModularSdk', () => ({ -// default: vi.fn(), -// }));
342-353: Use fake timers to eliminate real-time sleeps and flakiness.Multiple tests wait ~1.1s for debounce. Switch to vi.useFakeTimers() and advance timers for speed and stability.
+vi.useFakeTimers(); ... -// Wait for debounced effect -await new Promise((r) => setTimeout(r, 1100)); +// Fast-forward debounce +await waitFor(() => { + vi.advanceTimersByTime(1100); +});Also ensure useIntentSdk mock returns the full shape ({ intentSdk, areModulesInstalled, isInstalling, installModules, isFetching, error?, clearError? }) across tests to avoid brittle destructuring.
715-726: Assert post-install flow where appropriate.You verify installModules is called. Consider also asserting UI state changes (e.g., button disabled while installing) or subsequent re-check of areModulesInstalled to catch regressions.
src/apps/pulse/components/Buy/Buy.tsx (1)
83-91: Hook integration is correct; consider UX follow-up.After installModules resolves, consider auto-rechecking readiness and opening preview to reduce extra click.
- if (!areModulesInstalled) { - await installModules(); - } else { + if (!areModulesInstalled) { + await installModules(); + // Optional: re-check and proceed automatically + // if (areModulesInstalledAfter) setPreviewBuy(true); + } else { setExInResp(expressIntentResponse); setPreviewBuy(true); }src/apps/pulse/hooks/useIntentSdk.ts (1)
10-22: Type surface: consider making payingTokens optional with a default.It simplifies callers and reduces guard complexity.
-interface IntentProps { - payingTokens: PayingToken[]; -} +interface IntentProps { + payingTokens?: PayingToken[]; +} ... -export default function useIntentSdk(props: IntentProps) { - const { payingTokens } = props; +export default function useIntentSdk(props: IntentProps) { + const { payingTokens = [] } = props;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.json(1 hunks)src/apps/pulse/components/App/HomeScreen.tsx(1 hunks)src/apps/pulse/components/Buy/Buy.tsx(1 hunks)src/apps/pulse/components/Buy/PreviewBuy.tsx(1 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(6 hunks)src/apps/pulse/hooks/useIntentSdk.ts(3 hunks)src/apps/pulse/hooks/useModularSdk.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/apps/pulse/hooks/useModularSdk.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(23-215)
src/apps/pulse/components/Buy/Buy.tsx (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(23-215)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(23-215)
src/apps/pulse/components/App/HomeScreen.tsx (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(23-215)
src/apps/pulse/hooks/useIntentSdk.ts (2)
src/apps/pulse/components/Buy/PayingToken.tsx (1)
PayingToken(18-96)src/apps/pulse/types/tokens.ts (1)
PayingToken(12-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (4)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
75-77: Hook invocation LGTM.useIntentSdk({ payingTokens }) matches the new API. No issues with this change.
Ensure payingTokens is non-empty before actions relying on chainId; otherwise useIntentSdk skips readiness checks.
src/apps/pulse/components/App/HomeScreen.tsx (1)
112-114: Hook wiring looks correct.Passing payingTokens into useIntentSdk aligns with the refactor. No issues here.
src/apps/pulse/hooks/useIntentSdk.ts (1)
206-214: Return surface LGTM.Naming and exposed fields are clear and cohesive.
package.json (1)
23-23: Local tarball reproducibility confirmed; modular-sdk remains in active use.The tarball is present and the intent-sdk imports are active across production code. However,
@etherspot/modular-sdkis still declared in package.json and actively mocked insrc/test-utils/setupTests.ts(lines 95–98), so it cannot be removed as suggested. The original review's recommendation to drop it is incorrect—it remains a required dependency for tests.Verify only that the
.tgzfile is committed to the repository and CI/CD has access to it; otherwise no changes are needed.Likely an incorrect or invalid review comment.
| // vi.mock('../../../hooks/useModularSdk', () => ({ | ||
| // default: vi.fn(), | ||
| // })); |
There was a problem hiding this comment.
Can we remove any dead code otherwise please leave a comment
src/apps/pulse/hooks/useIntentSdk.ts
Outdated
| if (userOpHash) | ||
| txnHash = await kit.getTransactionHash(userOpHash, chainId); | ||
| // eslint-disable-next-line no-console | ||
| console.log('response from tx kit: ', response, userOpHash, txnHash); |
There was a problem hiding this comment.
Please replace this with debug log utility
src/apps/pulse/hooks/useIntentSdk.ts
Outdated
| .then((res) => { | ||
| if (res) { | ||
| // eslint-disable-next-line no-console | ||
| console.log('isWalletReadyForPulse: ', res); |
There was a problem hiding this comment.
Please remove or comment out with a note
src/apps/pulse/hooks/useIntentSdk.ts
Outdated
| setIsFetching(false); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('err:: ', err); |
There was a problem hiding this comment.
Can we make "err::" more relevant please
src/apps/pulse/hooks/useIntentSdk.ts
Outdated
| const ok = await sendTransactions(res, chainId); | ||
| setAreModulesInstalled(ok); | ||
| } catch (err) { | ||
| console.error('Installation failed:: ', err); |
There was a problem hiding this comment.
| console.error('Installation failed:: ', err); | |
| console.error('Pulse trading - Installation failed: ', err); |
Remember to make the messages relevant to everyone if you are leaving them in
package.json
Outdated
| "slick-carousel": "1.8.1", | ||
| "styled-components": "6.1.1", | ||
| "terser-webpack-plugin": "5.3.14", | ||
| "type-fest": "^0.16.0", |
There was a problem hiding this comment.
Please fix this version in place like the others
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
148-173: Major: Module state isn't revalidated when chain changes.The guard
!areModulesInstalled &&on line 150 prevents re-checking when the user switches chains. If modules are installed on Chain A but not on Chain B, switching to Chain B will incorrectly report modules as installed.Apply this diff to track the last-checked chain and revalidate on change:
+ const [lastCheckedChainId, setLastCheckedChainId] = useState<number | null>(null); + useEffect(() => { + if (!intentSdk || !payingTokens?.length) return; + const { chainId } = payingTokens[0]; + + // Re-check if chain changed or not yet checked + if (lastCheckedChainId !== null && lastCheckedChainId === chainId && areModulesInstalled) { + return; + } + - if ( - !areModulesInstalled && - intentSdk && - payingTokens && - payingTokens.length > 0 - ) { - const { chainId } = payingTokens[0]; setIsFetching(true); intentSdk .isWalletReadyForPulse(chainId) .then((res) => { if (res) { setAreModulesInstalled(true); } else { setAreModulesInstalled(false); } + setLastCheckedChainId(chainId); setIsFetching(false); }) .catch((err) => { console.error('Error on checking the pulse wallet: ', err); setIsFetching(false); setAreModulesInstalled(false); + setLastCheckedChainId(chainId); }); - } - }, [intentSdk, payingTokens, areModulesInstalled]); + }, [intentSdk, payingTokens?.[0]?.chainId]);
🧹 Nitpick comments (1)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
343-354: Consider extracting repeated mock setup into a test helper.The same
useIntentSdkmock pattern withintentSdk.expressIntentis repeated across five test cases. This duplication makes the tests harder to maintain.Consider creating a helper function in the test file:
const mockUseIntentSdkWithExpress = (overrides = {}) => { (useIntentSdk as any).mockReturnValue({ intentSdk: { expressIntent: vi.fn().mockResolvedValue({ intentHash: '0xIntentHash123456789', bids: [{ bidHash: '0xBidHash123456789' }], }), }, areModulesInstalled: true, isInstalling: false, installModules: vi.fn(), isFetching: false, ...overrides, }); };Then use it in tests:
mockUseIntentSdkWithExpress({ isFetching: true });Also applies to: 372-383, 415-426, 466-477, 723-734
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(2 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(5 hunks)src/apps/pulse/hooks/useIntentSdk.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.ts
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.tssrc/apps/pulse/components/Buy/tests/Buy.test.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.tssrc/apps/pulse/components/Buy/tests/Buy.test.tsx
🧬 Code graph analysis (2)
src/apps/pulse/hooks/useIntentSdk.ts (1)
src/apps/pulse/types/tokens.ts (1)
PayingToken(12-21)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(23-204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (6)
src/apps/pulse/hooks/useIntentSdk.ts (5)
10-21: Type definitions look good.The
IntentPropsandTransactionsinterfaces are well-structured and appropriately scoped for internal use.
23-24: LGTM—parameterized hook enables per-chain module checks.The new signature properly accepts
payingTokensto enable chain-specific module installation state.
38-41: State initialization is appropriate.The module installation state flags are correctly initialized and typed.
175-189: Logic is sound, but depends on sendTransactions fix.The
installModulesimplementation correctly uses async/await with proper error handling. However, line 182 relies on the return value fromsendTransactions, which currently always returnstrue(see previous comment). Once that issue is fixed, this function will correctly reflect installation status.Ensure the
sendTransactionsfix is applied so this function can properly detect failures.
195-203: Expanded return surface is well-structured.The hook now properly exposes module installation state and actions alongside the existing SDK instance.
package.json (1)
85-85: type-fest 0.16.0 is outdated but upgrading to 5.x is not feasible without broader changes—verify if upgrade is needed.type-fest 0.16.0 is indeed from 2020 and current versions reach 5.2.0. However, upgrading to 5.x breaks compatibility with webpack-dev-server (which constrains type-fest to
>=0.17.0 <5.0.0), requires TypeScript >= 5.9 (your project uses 5.5.3), Node >= 20, and mandates ESM-only consumption. Since type-fest is not directly used in the codebase (only as a transitive dependency), and other dependencies already pull in 0.20.2–0.21.3, consider whether upgrading within the 0.x range provides sufficient benefit to justify the maintenance effort, or if this is a lower-priority cleanup task.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/apps/pulse/hooks/useIntentSdk.ts (2)
45-74: MakesendTransactionsresult meaningful and guard batch access.Two problems here:
response.batches[batchName]will throw if the batch is missing.- You always return
true, soinstallModulescannot distinguish success from a silent failure (e.g. empty/missinguserOpHash).Consider this safer version:
const sendTransactions = async ( transactions: Transactions[], chainId: number ) => { try { - let txnHash; const batchName = 'pulse-install-modules'; for (let i = 0; i < transactions.length; i += 1) { kit .transaction({ to: transactions[i].target, data: transactions[i].calldata, chainId, }) .name({ transactionName: transactions[i].action }) .addToBatch({ batchName }); } const response = await kit.sendBatches({ onlyBatchNames: [batchName] }); - const userOpHash = - response.batches[batchName].chainGroups?.[chainId]?.userOpHash ?? ''; - if (userOpHash) { - txnHash = await kit.getTransactionHash(userOpHash, chainId); - transactionDebugLog('Install modules txn hash: ', txnHash); - } - return true; + const batch = response?.batches?.[batchName]; + const userOpHash = batch?.chainGroups?.[chainId]?.userOpHash ?? ''; + if (userOpHash) { + const txnHash = await kit.getTransactionHash(userOpHash, chainId); + transactionDebugLog('Install modules txn hash: ', txnHash); + } + return Boolean(userOpHash); } catch (err) { console.error('err on sending Install modules: ', err); return false; } };This lets
installModulescorrectly reflect whether anything was actually submitted.
152-178: Re-check modules per chain and avoid repeated inflight checks.Two related issues in this effect:
Stale per‑chain state:
Because of!areModulesInstalledin the guard, once a check succeeds on one chain,areModulesInstalledstaystrueand you never re‑runisWalletReadyForPulsewhenpayingTokens[0].chainIdchanges. That can mis-report modules as installed on a new chain.Effect runs on every render:
transactionDebugLogis a new function on each render (peruseTransactionDebugLogger), so it keeps this effect “dirty”. While the!areModulesInstalledguard limits calls post‑success, you can still fire multiple concurrent checks before the first resolves.A more robust pattern:
- useEffect(() => { - if ( - !areModulesInstalled && - intentSdk && - payingTokens && - payingTokens.length > 0 - ) { - const { chainId } = payingTokens[0]; + useEffect(() => { + const chainId = payingTokens?.[0]?.chainId; + if (!intentSdk || !chainId) return; + + // Optional: track last-checked chain or skip if already fetching + setIsFetching(true); setIsFetching(true); intentSdk .isWalletReadyForPulse(chainId) .then((res) => { transactionDebugLog('Pulse wallet modules installed: ', res); - if (res) { - setAreModulesInstalled(true); - } else { - setAreModulesInstalled(false); - } + setAreModulesInstalled(res); setIsFetching(false); }) .catch((err) => { console.error('Error on checking the pulse wallet: ', err); setIsFetching(false); setAreModulesInstalled(false); }); - } - }, [intentSdk, payingTokens, areModulesInstalled, transactionDebugLog]); + }, [intentSdk, payingTokens?.[0]?.chainId, transactionDebugLog]);Optionally, you can also:
- Add an
isFetchingguard in theifcondition to prevent overlapping checks, or- Make
transactionDebugLogstable viauseCallbackinsideuseTransactionDebugLoggerso it doesn’t retrigger the effect every render.
🧹 Nitpick comments (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
180-196:installModulesflow is solid after the async/await refactor.The guards on
payingTokens/intentSdk, the async/await usage, and the way you wireareModulesInstalled/isInstallingall look good and align with the Transaction Kit flow. OncesendTransactionsreturns a meaningful boolean, this path should behave predictably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/hooks/useIntentSdk.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.ts
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.ts
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.ts
🧬 Code graph analysis (1)
src/apps/pulse/hooks/useIntentSdk.ts (2)
src/apps/pulse/types/tokens.ts (1)
PayingToken(12-21)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (3)
src/apps/pulse/hooks/useIntentSdk.ts (3)
9-26: Hook props and local types look consistent.The
IntentProps/Transactionsinterfaces and the newuseIntentSdk(props: IntentProps)signature are coherent with the existingPayingTokenshape and the Intent SDK usage.
39-44: New module state and debug logger wiring look good.
areModulesInstalled,isInstalling,isFetching, andtransactionDebugLogfit the new hook responsibilities and are used consistently later in the file.
202-210: Extended hook return shape is coherent with new responsibilities.Exposing
installModules,areModulesInstalled,isInstalling, andisFetchingalongside the existing SDK/error fields matches how the consumers are expected to drive the UI.
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit