Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In `@docker-compose.testing.yml`:
- Around line 6-36: The testing docker-compose is missing
NEXT_PUBLIC_ENVIRONMENT causing the app to fall back to "development"; add
NEXT_PUBLIC_ENVIRONMENT: staging to the environment block in
docker-compose.testing.yml (place it before the labels: section) so the value
used by src/components/layout/environment-badge.tsx and the enum in src/env.js
correctly identifies the testing deployment as staging.
In `@drizzle/0001_cold_leader.sql`:
- Around line 12-23: The migration currently derives category.type using
COALESCE(MAX(t."type"), 'expense') in the INSERT SELECT (see INSERT INTO
"category" ... SELECT ... COALESCE(MAX(t."type"), 'expense') as "type" FROM
"transaction" t GROUP BY t."user_id", t."category"), which can pick
alphabetically highest type (e.g., 'transfer') and produce surprising results;
change the SELECT to instead either always set the type to the explicit default
'expense' for every created category, or replace the MAX(...) expression with a
mode/most-frequent computation (e.g., a subquery or window function that counts
types per user+category and selects the most frequent) so the "type" column
reflects the intended default or the actual majority transaction type.
In `@src/app/transactions/columns.tsx`:
- Around line 63-72: The cell renderer function for the transactions table
accesses row.original.wallet.name without guarding against wallet being
undefined; update the cell (the cell: ({ row }) => { ... } function) to use
optional chaining (row.original.wallet?.name) or a null check and provide a safe
fallback string (e.g., "Unknown wallet" or empty string) so no runtime error
occurs when wallet is missing; ensure the displayed fallback is used in the
<span> that currently renders row.original.wallet.name and adjust any TypeScript
types if needed.
- Around line 40-58: The cell renderer for transactions reads transaction_date
into dt using DateTime.fromJSDate/fromISO/fromMillis but never checks
dt.isValid; update the cell function (the block that assigns dt in the cell: ({
row }) => { ... }) to verify dt.isValid after parsing and before calling
dt.toLocaleString(DateTime.DATE_MED), and if invalid return the same fallback
(<div className="w-[100px] font-medium">-</div>); ensure every branch that sets
dt (including the toISOString branch) is followed by a single validity check
rather than calling toLocaleString on a possibly invalid DateTime.
In `@src/app/transactions/data-table-pagination.tsx`:
- Around line 72-107: The four pagination buttons currently reuse t("back") and
t("next") which makes screen reader labels ambiguous; update the <span
className="sr-only"> text for the first/previous/next/last controls to use
distinct translation keys (e.g. t("first_page") for the ChevronsLeft/button
calling table.setPageIndex(0), t("previous_page") for the ChevronLeft/button
calling table.previousPage(), t("next_page") for the ChevronRight/button calling
table.nextPage(), and t("last_page") for the ChevronsRight/button calling
table.setPageIndex(pageCount - 1)); also add those translation keys (first_page,
previous_page, next_page, last_page) to your i18n translations.
In `@src/app/transactions/page.tsx`:
- Around line 86-97: The Select for wallet filtering (Select, SelectTrigger,
SelectValue, SelectContent, SelectItem) lacks an option to clear the filter; add
a top SelectItem representing "All wallets" (use tGeneral("all_wallets") for its
label) with a non-empty sentinel value (e.g. "all") and update the selection
mapping so that when the sentinel is chosen you set selectedWalletId to
undefined (and when rendering the Select pass selectedWalletId ?? "all"). Ensure
walletOptions rendering remains unchanged and handle the sentinel in any query
logic that reads selectedWalletId.
- Line 43: The current creation of formattedDate using
selectedDate.toISOString() converts the date to UTC and can shift the day for
users in non-UTC timezones; change the formatting to use local date semantics
instead (e.g., use Luxon like
DateTime.fromJSDate(selectedDate).toFormat('yyyy-MM-dd') or construct YYYY-MM-DD
from selectedDate.getFullYear(), getMonth()+1, getDate()) so formattedDate
reflects the user's local selected day; update the code that sets formattedDate
(the selectedDate -> formattedDate logic in page.tsx) to use the chosen
local-format approach.
In `@src/components/common/setup-modal.tsx`:
- Around line 529-537: Add accessible labels to the icon-only delete buttons by
giving the Button components an aria-label (or visually-hidden screen-reader
text) that describes the action and target (e.g., "Delete category
{category.name}" or "Delete category"). Update both instances where the Trash2
icon button is used (the button that calls handleDeleteCategory(category.id) and
uses deleteCategoryMutation.isPending) to include aria-label text and ensure the
label conveys the target; keep the existing onClick, disabled logic and
className intact.
- Around line 260-266: The useEffect that auto-loads defaults in setup-modal
(the effect using stepper.current.id, categoriesLoaded and calling
handleLoadDefaultCategories) misses firing when the categories fetch resolves
after the user lands on the categories step; include categories in the
dependency array and keep the guard (stepper.current.id === "categories" &&
!categoriesLoaded && categories && categories.length === 0) so the effect
re-runs when categories changes (or use categories?.length === 0) and remove the
manual eslint-disable to ensure correct dependencies are tracked and the default
load runs when fetched categories become an empty array.
In `@src/components/layout/bottom-bar.tsx`:
- Around line 45-56: The Link inside DropdownMenuItem that currently navigates
to "/categories" should be changed to point to the settings page with the
category query param; update the href on the Link component (inside the
DropdownMenuItem in bottom-bar.tsx) from "/categories" to
"/settings?category=categories" so the Settings route handles category
management; leave the icon (Settings) and the i18n call t("settings") as-is.
In `@src/components/settings/categories-settings.tsx`:
- Around line 340-343: The delete confirmation copy in the CategoriesSettings
component is hard-coded; replace the two English strings around
categoryToDelete?.name with translated messages pulled from your i18n system
(e.g., the existing t/useTranslations hook used in this codebase): add
translation keys like "settings.deleteCategoryConfirmation" (e.g., "Are you sure
you want to delete the category \"{name}\"?") and
"settings.deleteCategoryWarning" (e.g., "This action cannot be undone."), then
render them with the translation function and interpolate
categoryToDelete?.name; update the locale files accordingly so i18n coverage
remains consistent.
- Around line 113-140: The CategoryItem currently uses a clickable <div> that
only reveals action buttons on hover, preventing keyboard users from accessing
edit/delete; change the root to a focusable element (preferably a <button> or
add tabIndex={0} role="button") and wire an onKeyDown handler to invoke onEdit
when Enter/Space is pressed; update the reveal CSS from group-hover:opacity-100
to also include focus (e.g., group-focus:opacity-100 or make the root a <button>
so :focus works) so actions appear on keyboard focus, and keep the inner Button
onClick handlers and their e.stopPropagation() behavior to prevent the root
activation when clicking/tabbing the action buttons (refer to CategoryItem,
onEdit, onDelete, and the group-hover class in the root and actions container).
In `@src/components/transactions/category-select.tsx`:
- Around line 38-52: The UI shows the "no categories" state before fetch
completes; update the useFetch call to also destructure isLoading (from
useFetch) alongside data/refetch, then change the empty-state logic to only show
the alert when isLoading is false and filteredCategories is empty: compute
filteredCategories from categoriesData and keep hasCategories =
(filteredCategories.length > 0), but gate rendering of the "no categories"
message with !isLoading && !hasCategories (referencing useFetch, categoriesData,
filteredCategories, hasCategories, and transactionType to locate the relevant
code).
- Around line 147-151: Guard the Enter-key handler against submitting while the
create-category mutation is in flight: check the mutation pending flag (e.g.,
createCategory.isLoading or createCategory.isPending) inside the onKeyDown
before calling handleCreateCategory and return early if it's true;
alternatively, add a boolean (like isCreating) used by both the createCategory
mutation and the onKeyDown to prevent re-entry when createCategory is pending,
and ensure handleCreateCategory itself also checks this flag to avoid duplicate
requests.
In `@src/components/transactions/edit-transaction-modal.tsx`:
- Around line 93-95: The current canSave guard uses selectedCategory and thus
blocks saving transfers (which don't render a CategorySelect) and also doesn't
require selectedSecondWallet; update the canSave expression (and sameWallet
check) to: require selectedCategory only when the transaction is not a transfer,
and require selectedSecondWallet when the transaction is a transfer; locate the
transfer indicator used in this component (e.g., isTransfer or transaction.type)
and change const sameWallet = selectedFirstWallet && selectedSecondWallet &&
selectedFirstWallet === selectedSecondWallet; then change const canSave =
selectedFirstWallet && date && amount !== 0 && !sameWallet && (isTransfer ?
selectedSecondWallet : selectedCategory) so transfers validate second wallet and
non-transfers validate category.
In `@src/components/transactions/transaction-item.tsx`:
- Around line 53-57: transaction.category.iconName is being unsafely cast to
IconName in TransactionItem (transaction-item.tsx) which can break rendering;
instead add a runtime guard: create or use a validation helper (e.g.,
isValidIconName(iconName): iconName is IconName) or a list/Set of allowed names
and only pass the value to <Icon> when the check passes, otherwise render a safe
fallback icon or null. Update the rendering branch that references
transaction.category.iconName to use this guard and remove the direct cast to
IconName so only validated values reach the Icon component.
In `@src/env.js`:
- Around line 13-15: The NODE_ENV z.enum currently only allows "development" and
"production", which will fail validation for test runs; update the enum in
src/env.js (the z.enum call that defines NODE_ENV) to include "test" or,
alternatively, adjust test scripts to set SKIP_ENV_VALIDATION=true before
running tests; specifically modify the NODE_ENV definition
(z.enum(["development","production"])...) to
z.enum(["development","production","test"]). Ensure any references to NODE_ENV
elsewhere accept "test" as a valid value.
In `@src/server/api/services/spendingsService.ts`:
- Line 14: The declared return type Promise<Transaction> for getSpendingsInRange
is wrong — the function actually returns an array of objects shaped like {
category: string | null, totalSpent: number }; update the signature on
getSpendingsInRange (and any related export/import) to return Promise<{
category: string | null; totalSpent: number }[]> or create and use a named
type/interface (e.g., SpendingSummary[]) and replace Promise<Transaction> with
Promise<SpendingSummary[]>; ensure SpendingOptions remains unchanged and adjust
any callers/types that expected Transaction accordingly.
In `@src/server/scripts/seed.ts`:
- Around line 137-145: The code assumes validCategories always has entries which
can make randomCategory undefined; update the block around
validCategories/randomCategory (referring to variables validCategories,
randomCategory, insertedCategories, transactionsToInsert) to guard against an
empty array: if validCategories.length === 0, either skip creating/pushing this
transaction (continue) or pick a safe fallback (e.g., a default category from
insertedCategories or set categoryId to null if allowed) and log a warning;
otherwise compute randomCategory normally and use randomCategory.id. Ensure the
chosen approach avoids the non-null assertion and prevents a runtime crash.
In `@TODO.md`:
- Line 3: Fix the typo in the TODO text "find a library for reshufling the cards
(swapy or https://github.com/sadmann7/sortable)" by changing "reshufling" to
"reshuffling" so the line reads "find a library for reshuffling the cards (swapy
or https://github.com/sadmann7/sortable)".
🧹 Nitpick comments (22)
src/components/ui/button.tsx (1)
22-23: Consider maintaining consistency with semantic design tokens.The
successvariant now uses hardcoded Tailwind colors (bg-green-700,hover:bg-green-800) while other variants likedestructive,warning, andinfouse semantic tokens with opacity modifiers (e.g.,hover:bg-destructive/90). This inconsistency could make future theming changes more difficult.If hardcoded colors are intentional for better control, consider applying the same pattern to other semantic variants for consistency, or document why
successdiffers.src/hooks/use-api.tsx (1)
99-108: Query key stability consideration.Including
options?.queryin thequeryKey(line 103) is correct for cache differentiation. However, be aware that if callers create a new query object on each render:// This creates a new object reference each render, potentially causing refetches useFetch("/api/items", { query: { filter: "active" } });React Query uses deep comparison for keys, so identical content should work correctly. To be safe, callers should memoize query objects or pass stable references.
Additionally, when
queryis undefined, the key becomes[url, undefined]which differs from[url]. This is generally fine but worth noting for consistency.💡 Optional: Filter undefined from queryKey
- queryKey: options?.queryKey ?? [url, options?.query], + queryKey: options?.queryKey ?? (options?.query ? [url, options.query] : [url]),src/server/db/category.ts (1)
1-31: Add a DB-level constraint fortype.
The comment restricts values to “income/expense”, but the column is unconstrained, so invalid values could slip in via direct DB writes or future services. Consider enforcing the allowed set with a CHECK/enum in the migration and keep API validation aligned.src/components/wallets/edit-wallet-modal.tsx (1)
320-328: Prefer a semantic button for the clickable description.
The span is interactive but not keyboard-focusable; using a<button type="button">improves accessibility without changing behavior.♿ Suggested tweak
- <span - className="text-sm text-muted-foreground cursor-pointer" - onClick={() => - dispatch({ - type: "SET_IS_SAVING_ACCOUNT", - payload: !state.isSavingAccount, - }) - } - > + <button + type="button" + className="text-sm text-muted-foreground cursor-pointer" + onClick={() => + dispatch({ + type: "SET_IS_SAVING_ACCOUNT", + payload: !state.isSavingAccount, + }) + } + > {t("saving_account_description")} - </span> + </button>src/app/transactions/columns.tsx (2)
11-34: Use proper typing for column parameter instead ofany.All header components use
column: anywhich loses type safety. Consider using the properColumntype from TanStack Table:import type { Column } from "@tanstack/react-table" const DateHeader = ({ column }: { column: Column<TransactionWithCategory, unknown> }) => { // ... }Alternatively, extract a shared type to avoid repetition across all header components.
110-113: Hardcoded locale may cause inconsistent formatting.The currency formatting uses hardcoded
"en-US"locale while the rest of the component usesnext-intlfor internationalization. Consider using the user's locale for consistent formatting, or use theformatCurrencyhelper from@/hooks/currenciesthat's already used intransaction-list.tsx.src/components/transactions/transaction-list.tsx (3)
172-182: Unsafe cast of iconName to IconName.The cast
as IconNameassumesiconNameis always a valid icon identifier. If the database contains an invalid or outdated icon name, theIconcomponent may fail to render properly.Consider either:
- Validating
iconNameagainst validIconNamevalues before rendering- Adding error boundary or fallback in case of invalid icon names
Suggested defensive approach
{transaction.category ? ( <div className="flex items-center gap-2"> - {transaction.category.iconName && ( - <Icon name={transaction.category.iconName as IconName} className="w-4 h-4 flex-shrink-0" /> - )} + {transaction.category.iconName ? ( + <Icon name={transaction.category.iconName as IconName} className="w-4 h-4 flex-shrink-0" /> + ) : null} <span className="truncate">{transaction.category.name}</span> </div>The current
&&check is fine for falsy values, but doesn't protect against invalid icon names.
142-144:isDeletingreflects global mutation state, not per-transaction.The
isDeletingprop passed toTransactionItemisremoveTransactionMutation.isPending, which istruefor any ongoing delete operation. This means all transaction items will show a deleting state when any single transaction is being deleted.Consider tracking which specific transaction ID is being deleted to only show the loading state on that item.
27-32: Consider sharingTransactionsResponsetype with server.The
TransactionsResponsetype is defined locally here but likely mirrors the server's response shape. Consider exporting this type from the server/API layer to ensure type consistency and avoid drift.docker-compose.yml (1)
6-8: Consider adding a default value forNEXT_PUBLIC_APP_URL.
NEXT_PUBLIC_APP_URLhas no default value unlikeNEXT_PUBLIC_ENVIRONMENT. If not set in the environment, this will be empty, which could cause issues with URL generation or redirects.Suggested fix
environment: - NEXT_PUBLIC_APP_URL: ${NEXT_PUBLIC_APP_URL} + NEXT_PUBLIC_APP_URL: ${NEXT_PUBLIC_APP_URL:-http://localhost:3000} NEXT_PUBLIC_ENVIRONMENT: ${NEXT_PUBLIC_ENVIRONMENT:-development}src/server/scripts/seed.ts (1)
128-146: Transaction description uses stale category names.The
categoryvariable (line 129) is selected from the oldcategoryListarrays (lines 79-80), but the actualcategoryIdis assigned frominsertedCategories. This creates a mismatch where the description may say "Paid Groceries" while the actual category is "Dining".Consider using
randomCategory.namefor consistency:♻️ Proposed fix
transactionsToInsert.push({ userId: demoUser.id, walletId: walletId, amount: amount, type: type, categoryId: randomCategory.id, - description: `${type === "income" ? "Received" : "Paid"} ${category}`, + description: `${type === "income" ? "Received" : "Paid"} ${randomCategory.name}`, transaction_date: date, });src/server/api/services/spendingsService.ts (1)
32-38: Transactions with null categoryId will be grouped under a singlenullcategory.Due to the
leftJoin, transactions without a category will returncategories.nameasnulland be grouped together. If this is intentional, consider adding a fallback label like "Uncategorized" for clarity in the UI.♻️ Optional: Add fallback for null category
const spendingData = await ctx.db .select({ - category: categories.name, + category: sql<string>`COALESCE(${categories.name}, 'Uncategorized')`.as("category"), totalSpent: sql<number>`sum(${transactions.amount})`.as("totalSpent"), }) .from(transactions) .leftJoin(categories, eq(transactions.categoryId, categories.id)) .where(and(...whereClause)) - .groupBy(categories.name); + .groupBy(sql`COALESCE(${categories.name}, 'Uncategorized')`);deploy-staging.sh (2)
69-70: Errors during database import are suppressed.Redirecting psql output to
/dev/nullhides any errors that might occur during the import, making debugging difficult if something goes wrong.♻️ Proposed fix to capture errors while suppressing notices
echo "Step 8: Applying production database data..." -cat "$DUMP_FILE" | docker exec -i "$STAGING_CONTAINER" psql -U "$STAGING_USER" "$STAGING_DB" > /dev/null +cat "$DUMP_FILE" | docker exec -i "$STAGING_CONTAINER" psql -U "$STAGING_USER" "$STAGING_DB" 2>&1 | grep -v "^NOTICE:" || trueAlternatively, log output to a file for later inspection:
cat "$DUMP_FILE" | docker exec -i "$STAGING_CONTAINER" psql -U "$STAGING_USER" "$STAGING_DB" > import.log 2>&1
46-47: Consider adding--no-ownerand--no-aclto pg_dump.If the staging database uses different role names than production, the dump's ownership/ACL statements may fail. These flags ensure a cleaner import.
♻️ Proposed fix
echo "Step 1: Cloning production database..." -docker exec "$PROD_CONTAINER" pg_dump -U "$PROD_USER" "$PROD_DB" > "$DUMP_FILE" +docker exec "$PROD_CONTAINER" pg_dump -U "$PROD_USER" --no-owner --no-acl "$PROD_DB" > "$DUMP_FILE"src/server/api/routers/statsRouter.ts (1)
19-39: Query parametercategoryis misleading since it expects a category ID.The parameter is named
categorybut is used as an ID in the filtereq(transactions.categoryId, category). Consider renaming tocategoryIdfor API clarity.♻️ Proposed fix
statsRouter.get("/spendings", authenticated, zValidator( "query", z.object({ startDate: z.coerce.date().optional(), endDate: z.coerce.date().optional(), walletId: z.string().optional(), - category: z.string().optional(), + categoryId: z.string().optional(), }), ), async (c) => { const { user } = await getUserData(c); - const { startDate, endDate, walletId, category } = c.req.valid("query"); + const { startDate, endDate, walletId, categoryId } = c.req.valid("query"); // ... - category ? eq(transactions.categoryId, category) : undefined, + categoryId ? eq(transactions.categoryId, categoryId) : undefined,src/components/settings/account-settings.tsx (1)
2-47: Avoid large commented-out JSX; track suspend-account work separately.
The TODO and commented block add noise. Consider removing the commented code and tracking the suspend-account feature via an issue or feature flag.If you want, I can draft an issue template or a feature-flag stub for the suspend-account flow.
src/app/transactions/data-table.tsx (1)
41-54: Reset pagination on filter changes + localize the default placeholder.
If a user filters while on a later page, the table can appear empty despite matching rows. Consider resettingpageIndexwhen filters change. Also, the defaultfilterPlaceholderis hard-coded English; consider using translations or requiring the caller to pass a localized string.Also applies to: 90-99
src/app/transactions/page.tsx (1)
28-28:PAGE_LIMITof 100 may be excessive for initial load.Fetching 100 transactions upfront when the DataTable displays only 10 rows per page (default
pageSizeindata-table.tsx) is inefficient. Consider aligning the fetch limit with the displayed page size or implementing server-side pagination.Also applies to: 49-57
src/server/api/routers/transactionsRouter.ts (2)
208-208: Update endpoint uses POST instead of PUT/PATCH.Using
POST /:idfor updates deviates from REST conventions wherePUTorPATCHwould be expected for modifying existing resources. This could cause confusion for API consumers.♻️ Suggested change to use PUT
-transactionsRouter.post("/:id", authenticated, zValidator("param", z.object({ +transactionsRouter.put("/:id", authenticated, zValidator("param", z.object({
233-238: Clarify the intent of conditional categoryId handling.The expression
categoryId || undefinedconverts empty strings toundefined, but the schema allowscategoryIdto bez.string().optional(). If the intent is to allow clearing a category (setting it tonull), the current logic won't support that becausecategoryId: ""becomescategoryId: undefined, which is then excluded from the update.If clearing categories should be supported, consider:
♻️ Suggested approach for explicit null clearing
- categoryId: z.string().optional(), + categoryId: z.string().nullable().optional(),- ...(categoryId !== undefined && { categoryId: categoryId || undefined }), + ...(categoryId !== undefined && { categoryId: categoryId || null }),drizzle/0001_cold_leader.sql (1)
26-26: Consider adding an index ontransaction.category_id.The new
category_idcolumn will likely be used in WHERE clauses and JOINs. Adding an index can improve query performance, especially as the transaction table grows.📈 Suggested index addition
--> statement-breakpoint CREATE INDEX "transaction_category_id_idx" ON "transaction" ("category_id");messages/en.json (1)
37-42: Duplicate translation keys across sections.Several keys are duplicated between
serviceandgeneralsections:
save(service.save, general.save)cancel(service.cancel, general.cancel)delete(service.delete, general.delete)back(service.back, general.back)next(service.next, general.next)Consider consolidating these to a single location to avoid maintenance overhead and potential inconsistencies.
Also applies to: 74-75
1 participant
Summary by CodeRabbit
Release Notes