Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughThis PR introduces a transaction synchronization flow between on-chain list contract operations and an off-chain indexer. It adds three new sync API methods, refactors contract methods to return transaction hashes instead of list results, and updates UI components to call the sync API after on-chain operations complete. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant Contract as List Contract
participant Wallet as Wallet
participant SyncAPI as Sync API
participant Indexer as Indexer
UI->>Contract: upvote({list_id})
Contract->>Wallet: Sign transaction
Wallet-->>Contract: Return tx outcome
Contract-->>UI: Return txHash
alt txHash exists & viewer signed in
UI->>SyncAPI: listUpvote(listId, txHash, senderId)
SyncAPI->>Indexer: POST sync request
Indexer-->>SyncAPI: Response
SyncAPI-->>UI: {success, message}
else
UI-->>UI: Skip sync
end
UI-->>UI: Show success toast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/entities/list/hooks/useListForm.ts (1)
42-48:awaiton the sync call delays navigation to/lists.Because
syncApi.listDeleteis awaited on Line 45, thepush("/lists")on Line 48 won't execute until the sync API responds (or the.catchswallows a timeout/network error). If the sync endpoint is slow or unreachable, the user will be stuck with no feedback.Consider firing the sync call without awaiting it, since the on-chain deletion is already confirmed and the sync result isn't used:
🔧 Proposed fix: fire-and-forget the sync call
.then(async ({ txHash }) => { // Sync deletion to indexer if (txHash && viewer.accountId) { - await syncApi.listDelete(id, txHash, viewer.accountId).catch(() => {}); + syncApi.listDelete(id, txHash, viewer.accountId).catch(() => {}); } push("/lists"); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entities/list/hooks/useListForm.ts` around lines 42 - 48, The await on syncApi.listDelete is blocking navigation; change the call in the then handler for txHash so it is fired-and-forget instead of awaited—invoke syncApi.listDelete(id, txHash, viewer.accountId) without await (e.g., prefix with void or drop await) and keep the existing .catch to swallow errors, then immediately call push("/lists"); this ensures on-chain deletion (txHash) is handled but navigation via push proceeds without waiting for the sync API.src/common/api/indexer/sync.ts (1)
9-274: Optional: Extract a shared helper to reduce boilerplate across all sync methods.Every method in
syncApirepeats the same fetch → checkresponse.ok→ parse JSON → catch pattern. A small helper likesyncPost(url, body?)could reduce ~15 lines per method to ~1-2 lines, making the file significantly more maintainable. This is a pre-existing concern that the new methods inherit.💡 Sketch of a shared helper
async function syncPost( url: string, body?: Record<string, unknown>, ): Promise<{ success: boolean; message?: string }> { try { const response = await fetch(url, { method: "POST", ...(body && { headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), }), }); if (!response.ok) { const error = await response.json().catch(() => ({})); console.warn(`Sync failed for ${url}:`, error); return { success: false, message: error?.error || "Sync failed" }; } const result = await response.json(); return { success: true, message: result.message }; } catch (error) { console.warn(`Sync failed for ${url}:`, error); return { success: false, message: String(error) }; } } // Usage: async listDelete(listId, txHash, senderId) { return syncPost( `${SYNC_API_BASE_URL}/api/v1/lists/${listId}/delete/sync`, { tx_hash: txHash, sender_id: senderId }, ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/api/indexer/sync.ts` around lines 9 - 274, The syncApi object contains many repetitive POST/fetch blocks (e.g., campaign, campaignDonation, campaignDelete, campaignRefund, campaignUnescrow, listDelete, listUpvote, listRemoveUpvote); extract a shared async helper (suggested name syncPost) that accepts a URL and optional body, performs the POST with Content-Type when body is provided, checks response.ok, parses JSON, logs errors including the URL and error details, and returns the unified { success, message } shape; then refactor each method in syncApi to call syncPost with the appropriate endpoint and body (e.g., listDelete(... ) => return syncPost(`${SYNC_API_BASE_URL}/api/v1/lists/${listId}/delete/sync`, { tx_hash: txHash, sender_id: senderId })) to remove duplicated fetch/response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/entities/list/components/ListCard.tsx`:
- Around line 46-71: Convert the possibly-string on_chain_id to a number before
passing to contract and sync calls in ListCard.tsx: replace uses of
dataForList?.on_chain_id with a normalized numeric id (e.g., const listId =
Number(dataForList?.on_chain_id) or similar) and pass listId into
listsContractClient.remove_upvote / listsContractClient.upvote and into
syncApi.listRemoveUpvote / syncApi.listUpvote, keeping the rest of the
promise/txHash handling intact; mirror the normalization used in
Number(listDetails.on_chain_id) from ListDetails.tsx to ensure the contract
receives a number.
In `@src/entities/list/components/ListDetails.tsx`:
- Around line 135-136: The code assigns onChainId via
Number(listDetails.on_chain_id) which can yield NaN for
undefined/null/non-numeric values; change the logic in ListDetails.tsx around
the onChainId declaration to parse and validate the value (e.g., use
parseInt(listDetails.on_chain_id, 10) or Number and then check
Number.isFinite/!Number.isNaN), provide a safe fallback or early return/error UI
when invalid, and ensure any downstream use (contract calls or sync API that
consume list_id) only runs when onChainId is a valid integer.
---
Nitpick comments:
In `@src/common/api/indexer/sync.ts`:
- Around line 9-274: The syncApi object contains many repetitive POST/fetch
blocks (e.g., campaign, campaignDonation, campaignDelete, campaignRefund,
campaignUnescrow, listDelete, listUpvote, listRemoveUpvote); extract a shared
async helper (suggested name syncPost) that accepts a URL and optional body,
performs the POST with Content-Type when body is provided, checks response.ok,
parses JSON, logs errors including the URL and error details, and returns the
unified { success, message } shape; then refactor each method in syncApi to call
syncPost with the appropriate endpoint and body (e.g., listDelete(... ) =>
return syncPost(`${SYNC_API_BASE_URL}/api/v1/lists/${listId}/delete/sync`, {
tx_hash: txHash, sender_id: senderId })) to remove duplicated fetch/response
handling.
In `@src/entities/list/hooks/useListForm.ts`:
- Around line 42-48: The await on syncApi.listDelete is blocking navigation;
change the call in the then handler for txHash so it is fired-and-forget instead
of awaited—invoke syncApi.listDelete(id, txHash, viewer.accountId) without await
(e.g., prefix with void or drop await) and keep the existing .catch to swallow
errors, then immediately call push("/lists"); this ensures on-chain deletion
(txHash) is handled but navigation via push proceeds without waiting for the
sync API.
Comment on lines
+46
to
+71
| listsContractClient | ||
| .remove_upvote({ list_id: dataForList?.on_chain_id }) | ||
| .then(async ({ txHash }) => { | ||
| if (txHash && viewer.accountId) { | ||
| await syncApi | ||
| .listRemoveUpvote(dataForList?.on_chain_id, txHash, viewer.accountId) | ||
| .catch(() => {}); | ||
| } | ||
| }) | ||
| .catch((error) => console.error("Error removing upvote:", error)); | ||
|
|
||
| dispatch.listEditor.handleListToast({ | ||
| name: truncate(dataForList?.name ?? "", 15), | ||
| type: ListFormModalType.DOWNVOTE, | ||
| }); | ||
| } else { | ||
| listsContractClient.upvote({ list_id: dataForList?.on_chain_id }); | ||
| listsContractClient | ||
| .upvote({ list_id: dataForList?.on_chain_id }) | ||
| .then(async ({ txHash }) => { | ||
| if (txHash && viewer.accountId) { | ||
| await syncApi | ||
| .listUpvote(dataForList?.on_chain_id, txHash, viewer.accountId) | ||
| .catch(() => {}); | ||
| } | ||
| }) | ||
| .catch((error) => console.error("Error upvoting:", error)); |
Contributor
There was a problem hiding this comment.
dataForList?.on_chain_id is passed directly without Number() conversion, unlike ListDetails.tsx.
In ListDetails.tsx (Line 135), the same value is converted via Number(listDetails.on_chain_id), but here the raw value from the API is passed to both the contract method (which expects list_id: number) and syncApi. If on_chain_id is a string from the API response, this could cause a type mismatch at the contract level.
Apply the same normalization used in ListDetails.tsx:
🔧 Proposed fix
const handleUpvote = (e: React.MouseEvent) => {
e.stopPropagation();
+ const onChainId = Number(dataForList?.on_chain_id);
+ if (Number.isNaN(onChainId)) return;
if (isUpvoted) {
listsContractClient
- .remove_upvote({ list_id: dataForList?.on_chain_id })
+ .remove_upvote({ list_id: onChainId })
.then(async ({ txHash }) => {
if (txHash && viewer.accountId) {
await syncApi
- .listRemoveUpvote(dataForList?.on_chain_id, txHash, viewer.accountId)
+ .listRemoveUpvote(onChainId, txHash, viewer.accountId)
.catch(() => {});
}
})
.catch((error) => console.error("Error removing upvote:", error));
// ...
} else {
listsContractClient
- .upvote({ list_id: dataForList?.on_chain_id })
+ .upvote({ list_id: onChainId })
.then(async ({ txHash }) => {
if (txHash && viewer.accountId) {
await syncApi
- .listUpvote(dataForList?.on_chain_id, txHash, viewer.accountId)
+ .listUpvote(onChainId, txHash, viewer.accountId)
.catch(() => {});
}
})
.catch((error) => console.error("Error upvoting:", error));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/entities/list/components/ListCard.tsx` around lines 46 - 71, Convert the
possibly-string on_chain_id to a number before passing to contract and sync
calls in ListCard.tsx: replace uses of dataForList?.on_chain_id with a
normalized numeric id (e.g., const listId = Number(dataForList?.on_chain_id) or
similar) and pass listId into listsContractClient.remove_upvote /
listsContractClient.upvote and into syncApi.listRemoveUpvote /
syncApi.listUpvote, keeping the rest of the promise/txHash handling intact;
mirror the normalization used in Number(listDetails.on_chain_id) from
ListDetails.tsx to ensure the contract receives a number.
Comment on lines
+135
to
+136
| const onChainId = Number(listDetails.on_chain_id); | ||
|
|
Contributor
There was a problem hiding this comment.
Number(listDetails.on_chain_id) can produce NaN if on_chain_id is missing or non-numeric.
If listDetails.on_chain_id is undefined, null, or a non-numeric string, Number() returns NaN, which would be passed as list_id to the contract and sync API. Consider adding an early guard:
🛡️ Proposed guard
const handleUpvote = () => {
const onChainId = Number(listDetails.on_chain_id);
+ if (Number.isNaN(onChainId)) return;
if (isUpvoted) {📝 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.
Suggested change
| const onChainId = Number(listDetails.on_chain_id); | |
| const onChainId = Number(listDetails.on_chain_id); | |
| if (Number.isNaN(onChainId)) return; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/entities/list/components/ListDetails.tsx` around lines 135 - 136, The
code assigns onChainId via Number(listDetails.on_chain_id) which can yield NaN
for undefined/null/non-numeric values; change the logic in ListDetails.tsx
around the onChainId declaration to parse and validate the value (e.g., use
parseInt(listDetails.on_chain_id, 10) or Number and then check
Number.isFinite/!Number.isNaN), provide a safe fallback or early return/error UI
when invalid, and ensure any downstream use (contract calls or sync API that
consume list_id) only runs when onChainId is a valid integer.
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
1 participant
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 by CodeRabbit
Release Notes