fix: resolve critical performance bug and fix typos #107
Conversation
|
@Kabouziane is attempting to deploy a commit to the JS Mastery Pro Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughEdits correct typos and minor issues across UI components and update data handling in bank actions. Account field is renamed to shareableId across usage and return types. Transaction fetching now aggregates paginated results via push and maps accountId from transaction.account_id. Minor variable/type-name typos are fixed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant BA as bank.actions.getTransactions
participant API as Banking API
UI->>BA: requestTransactions(user, dateRange)
rect rgba(200,230,255,0.3)
note over BA: Initialize empty transactions[]
loop For each page until end
BA->>API: fetchTransactions(cursor/params)
API-->>BA: response { added[], next_cursor }
BA->>BA: transactions.push(...map(added){ accountId = account_id, ... })
BA->>BA: update cursor
end
end
BA-->>UI: aggregated transactions[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/actions/bank.actions.ts (4)
24-53: Guard against undefined banks when using Promise.allIf getBanks returns undefined/null, Promise.all(banks?.map(...)) will throw. Default to [] to avoid a crash.
- const accounts = await Promise.all( - banks?.map(async (bank: Bank) => { + const accounts = await Promise.all( + (banks ?? []).map(async (bank: Bank) => { // get each account info from plaid const accountsResponse = await plaidClient.accountsGet({ access_token: bank.accessToken, }); ... }) );
104-115: Add shareableId to getAccount’s returned accountPrevents undefined Copy title in BankCard when used with single-account data.
const account = { id: accountData.account_id, availableBalance: accountData.balances.available!, currentBalance: accountData.balances.current!, institutionId: institution.institution_id, name: accountData.name, officialName: accountData.official_name, mask: accountData.mask!, type: accountData.type as string, subtype: accountData.subtype! as string, appwriteItemId: bank.$id, + shareableId: bank.shareableId, };
150-179: Critical: transactionsSync pagination missing cursor — risk of infinite loop or duplicate pagesPlaid transactions/sync requires passing the cursor returned by the prior call to fetch subsequent pages. Without it, you can repeatedly receive the same first page when has_more is true.
Apply the following cursor-based pagination and add a safety cap:
export const getTransactions = async ({ accessToken, }: getTransactionsProps) => { - let hasMore = true; - let transactions: any = []; + let hasMore = true; + let cursor: string | undefined = undefined; + let transactions: any[] = []; try { // Iterate through each page of new transaction updates for item while (hasMore) { - const response = await plaidClient.transactionsSync({ - access_token: accessToken, - }); + const response = await plaidClient.transactionsSync({ + access_token: accessToken, + cursor, + }); const data = response.data; - transactions.push(...response.data.added.map((transaction) => ({ + transactions.push( + ...data.added.map((transaction) => ({ id: transaction.transaction_id, name: transaction.name, paymentChannel: transaction.payment_channel, - type: transaction.payment_channel, + type: transaction.payment_channel, // see note below to verify semantics accountId: transaction.account_id, amount: transaction.amount, pending: transaction.pending, category: transaction.category ? transaction.category[0] : "", date: transaction.date, - image: transaction.logo_url, - }))); + image: transaction.logo_url, + })) + ); - hasMore = data.has_more; + hasMore = data.has_more; + cursor = data.next_cursor; } return parseStringify(transactions); } catch (error) { - console.error("An error occurred while getting the accounts:", error); + console.error("An error occurred while getting the transactions:", error); } };Optionally, add a max-pages guard to avoid pathological loops:
let page = 0; const MAX_PAGES = 100; while (hasMore && page < MAX_PAGES) { ... page++; }
165-176: Ensuretyperemains “credit” | “debit” to preserve UI logicThe
typeproperty is used in TransactionsTable.tsx (lines 47–48) to drive debit/credit styling and filtering:
- const isDebit = t.type === 'debit';
- const isCredit = t.type === 'credit';
Mapping
typetotransaction.payment_channel(e.g., “online”, “in_store”, etc.) breaks this logic. Please update the mapping in lib/actions/bank.actions.ts (lines 165–176) so that:
paymentChannelremainstransaction.payment_channeltypeis derived as'debit'or'credit'For example, you could use the sign of
transaction.amount(negative →'debit', positive →'credit') or another Plaid field if available.Locations to update:
- lib/actions/bank.actions.ts:165–176 — replace
type: transaction.payment_channel,
with logic to output'debit'|'credit'.Verify downstream components (e.g., TransactionsTable.tsx) and any filters that depend on
t.type.components/BankDropdown.tsx (1)
43-46: defaultValue/value mismatch breaks initial selectionSelectItem value uses appwriteItemId, but defaultValue uses selected.id. This prevents the initial account from being selected in the UI.
- <Select - defaultValue={selected.id} + <Select + defaultValue={selected?.appwriteItemId} onValueChange={(value) => handleBankChange(value)} > ... - <SelectItem - key={account.id} - value={account.appwriteItemId} + <SelectItem + key={account.id} + value={account.appwriteItemId} className="cursor-pointer border-t" >Also applies to: 67-70
🧹 Nitpick comments (6)
components/MobileNav.tsx (2)
45-47: Remove redundant outer SheetClose aroundYou already wrap each with SheetClose, so the outer SheetClose around the entire
is redundant and may close the sheet when clicking any empty area inside the nav. Keep the per-link SheetClose only.- <div className="mobilenav-sheet"> - <SheetClose asChild> - <nav className="flex h-full flex-col gap-6 pt-16 text-white"> + <div className="mobilenav-sheet"> + <nav className="flex h-full flex-col gap-6 pt-16 text-white"> {sidebarLinks.map((item) => { const isActive = pathname === item.route || pathname.startsWith(`${item.route}/`) return ( <SheetClose asChild key={item.route}> <Link href={item.route} key={item.label} className={cn('mobilenav-sheet_close w-full', { 'bg-bank-gradient': isActive })} > ... </Link> </SheetClose> ) })} - </nav> - </SheetClose> + </nav>Also applies to: 74-74
51-53: Drop the inner key on Link to avoid duplicate keysThe key on the SheetClose is sufficient. The inner key on Link is redundant and uses a different key source (label), which can be confusing when debugging reconciliation.
- <SheetClose asChild key={item.route}> - <Link href={item.route} key={item.label} + <SheetClose asChild key={item.route}> + <Link href={item.route} className={cn('mobilenav-sheet_close w-full', { 'bg-bank-gradient': isActive })} >lib/actions/bank.actions.ts (3)
48-49: Propagate shareableId consistently in account objectsgetAccounts now exposes shareableId. For API consistency with components like BankCard, add shareableId to the single-account response too (see below patch on Lines 104–115).
141-146: Nit: correct error log contextMessage says “getting the accounts” inside getInstitution; adjust to “institution” for accurate logs.
- } catch (error) { - console.error("An error occurred while getting the accounts:", error); - } + } catch (error) { + console.error("An error occurred while getting the institution:", error); + }
182-184: Nit: correct error log contextThis is getTransactions; log should say “transactions” not “accounts”.
- console.error("An error occurred while getting the accounts:", error); + console.error("An error occurred while getting the transactions:", error);components/BankDropdown.tsx (1)
50-57: Null-safe render of selected account labelIf no accounts are present, selected is null; guard rendering to avoid runtime errors.
- <p className="line-clamp-1 w-full text-left">{selected.name}</p> + <p className="line-clamp-1 w-full text-left">{selected?.name ?? 'No accounts'}</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/BankCard.tsx(1 hunks)components/BankDropdown.tsx(1 hunks)components/MobileNav.tsx(2 hunks)components/Sidebar.tsx(1 hunks)lib/actions/bank.actions.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/actions/bank.actions.ts (1)
lib/utils.ts (1)
parseStringify(79-79)
🔇 Additional comments (5)
components/MobileNav.tsx (2)
23-23: Fix: width class corrected — LGTMThanks for correcting the container width to w-full. No further issues spotted in this hunk.
72-72: Stray content removed — LGTMThe previous stray text inside the sheet appears removed. Looks good.
components/Sidebar.tsx (1)
11-11: Typo fix in prop type name — LGTMRenaming SiderbarProps → SidebarProps improves clarity; no runtime impact.
components/BankCard.tsx (1)
61-61: Field rename to shareableId — LGTM, but ensure data parity across fetchersThis aligns with getAccounts returning shareableId. Verify that getAccount also includes shareableId to avoid Copy rendering undefined on views that use single-account data.
Would you like me to add shareableId to getAccount? I’ve proposed a patch in lib/actions/bank.actions.ts.
components/BankDropdown.tsx (1)
29-30: State setter rename — LGTMsetSelected replaces setSeclected; usage updated correctly.
| const searchParams = useSearchParams(); | ||
| const router = useRouter(); | ||
| const [selected, setSeclected] = useState(accounts[0]); | ||
| const [selected, setSelected] = useState(accounts[0]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty accounts in initial state
If accounts is empty, useState(accounts[0]) will set undefined and subsequent selected.name access will throw. Default to null or the first account if present.
- const [selected, setSelected] = useState(accounts[0]);
+ const [selected, setSelected] = useState<Account | null>(accounts[0] ?? null);📝 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.
| const [selected, setSelected] = useState(accounts[0]); | |
| const [selected, setSelected] = useState<Account | null>(accounts[0] ?? null); |
🤖 Prompt for AI Agents
In components/BankDropdown.tsx around line 24, the initial useState(accounts[0])
can be undefined when accounts is empty; change the initializer to
accounts.length > 0 ? accounts[0] : null, update the state type to allow null,
and add guards (or optional chaining) wherever selected is used (e.g.,
selected?.name) so the component handles an empty accounts array safely.
1 participant
Summary by CodeRabbit