Frontend/tix61 bill splitting#91
Conversation
Reviewer's GuideThis PR implements a bill-splitting feature by updating front-end dependencies, adding a summary dashboard to the Trip page, creating a dedicated bill-splitting page with tabbed sections for expense history, balances, and settlements, and introducing Formik-powered dialog forms with Zod validation. Sequence Diagram for Adding an ExpensesequenceDiagram
actor User
participant AddExpenseDialog
participant GroupExpensesPage
participant BackendAPI
User->>AddExpenseDialog: Fills expense details (description, amount, split method, shares)
User->>AddExpenseDialog: Clicks "Submit"
AddExpenseDialog->>GroupExpensesPage: onSubmit(expenseData)
GroupExpensesPage->>BackendAPI: POST /api/v1/groups/{id}/expenses with payload
BackendAPI-->>GroupExpensesPage: Expense creation status
GroupExpensesPage->>AddExpenseDialog: Closes dialog
GroupExpensesPage->>GroupExpensesPage: Refreshes trip data / updates UI
Sequence Diagram for Adding a SettlementsequenceDiagram
actor User
participant SettlementsTab
participant AddSettlementDialog
participant GroupExpensesPage
participant BackendAPI
User->>SettlementsTab: Selects a user to settle with
SettlementsTab->>AddSettlementDialog: Opens dialog (prefills debtor, creditor, amount)
User->>AddSettlementDialog: Confirms settlement amount
User->>AddSettlementDialog: Clicks "Confirm"
AddSettlementDialog->>GroupExpensesPage: onSubmit(settlementData) (via SettlementsTab prop)
GroupExpensesPage->>BackendAPI: POST /api/v1/groups/{id}/settlements with payload
BackendAPI-->>GroupExpensesPage: Settlement creation status
GroupExpensesPage->>AddSettlementDialog: Closes dialog
GroupExpensesPage->>GroupExpensesPage: Refreshes trip data / updates UI
Sequence Diagram for Loading Bill Splitting Page DatasequenceDiagram
participant GroupExpensesPage
participant BackendAPI
GroupExpensesPage->>BackendAPI: GET /api/v1/groups/{id}/balance/balances
BackendAPI-->>GroupExpensesPage: Group balance data
GroupExpensesPage->>BackendAPI: GET /api/v1/groups/{id}/expenses
BackendAPI-->>GroupExpensesPage: Expenses data
GroupExpensesPage->>BackendAPI: GET /api/v1/groups/{id}/balance/min
BackendAPI-->>GroupExpensesPage: Minimum settlements data
GroupExpensesPage->>BackendAPI: GET /api/v1/groups/{id}/settlements
BackendAPI-->>GroupExpensesPage: Settlements history data
GroupExpensesPage->>GroupExpensesPage: Updates state and renders UI
Entity Relationship Diagram for Bill Splitting Data StructureserDiagram
TRIP ||--o{ EXPENSE : "has"
TRIP ||--o{ SETTLEMENT : "has"
USER ||--o{ EXPENSE : "paid by"
USER ||--o{ SETTLEMENT : "is debtor in"
USER ||--o{ SETTLEMENT : "is creditor in"
EXPENSE ||--o{ EXPENSE_SHARE : "details in"
TRIP {
int id PK
string name
}
USER {
int id PK
string email
}
EXPENSE {
int id PK
int tripId FK "(groupId)"
int paidByUserId FK
string description
decimal totalAmount
string splitType "ENUM('AMOUNT', 'SHARES', 'PERCENTAGE', 'EQUALLY')"
datetime expenseDate
}
EXPENSE_SHARE {
int expenseId FK
int userId FK
decimal shareValue "(amount, percentage, or share count)"
boolean included
}
SETTLEMENT {
int id PK
int tripId FK "(groupId)"
int debtorId FK
int creditorId FK
decimal amount
datetime paymentDate
}
Class Diagram for New Frontend Components and SchemasclassDiagram
class GroupExpensesPage {
-trip: TripDetails
-error: string | null
-groupBalance: object[]
-expenses: object[]
-settlements: object[]
-settlementsHistory: object[]
-loading: boolean
-showDialog: boolean
-activeTab: string
+fetchData()
+handleExpenseSubmit(values: object)
+handleSettlementSubmit(values: object)
+render()
}
class ExpensesDashboard {
-trip: TripDetails
-error: string | null
-groupBalance: object[]
+fetchGroupExpenses()
+render()
}
class AddExpenseDialog {
+open: boolean
+setOpen(boolean)
+trip: TripDetails
+onSubmit(values: object, helpers: object)
+initialValues: object
+render()
}
class AddSettlementDialog {
+open: boolean
+onOpenChange(boolean)
+userEmail: string
+settlementType: string
+amount: number
+maxAmount: number
+debtorId: number
+creditorId: number
+groupId: number
+onSubmit(values: object)
+render()
}
class ExpensesHistoryTab {
+showDialog: boolean
+setShowDialog(boolean)
+trip: TripDetails
+initialValues: object
+onSubmit(values: object)
+paginatedExpenses: object[]
+expenses: object[]
+ITEMS_PER_PAGE: number
+render()
}
class BalancesTab {
+trip: TripDetails
+balanceMap: Map
+render()
}
class SettlementsTab {
+settlements: object[]
+trip: TripDetails
+currentUserId: number
+history: object[]
+onSubmit(values: object)
-showDialog: boolean
-selectedUser: object | null
-selectedAmount: number
-settlementType: string | null
+handleOpenSettlement(userId, amount, type)
+findSettlement(userId)
+render()
}
class ExpensesHistory {
+paginatedExpenses: object[]
+expenses: object[]
+trip: TripDetails
+ITEMS_PER_PAGE: number
+render()
}
class Checkbox {
+React.forwardRef()
+render()
}
class expenseFormSchema {
+description: string
+totalAmount: number
+splitMethod: enum('equal', 'percentage', 'amount', 'share')
+shares: ShareItem[]
}
class ShareItem {
+userId: number
+included: boolean
+value: number | undefined | null
}
class settlementFormSchema {
+amount: number
+maxAmountValidation: number
}
GroupExpensesPage --> ExpensesHistoryTab
GroupExpensesPage --> BalancesTab
GroupExpensesPage --> SettlementsTab
GroupExpensesPage ..> AddExpenseDialog : uses
GroupExpensesPage ..> AddSettlementDialog : uses indirectly via SettlementsTab
ExpensesHistoryTab ..> AddExpenseDialog : uses
ExpensesHistoryTab ..> ExpensesHistory : uses
SettlementsTab ..> AddSettlementDialog : uses
AddExpenseDialog ..> expenseFormSchema : validates against
AddSettlementDialog ..> settlementFormSchema : validates against
TripPage o-- ExpensesDashboard : contains
GroupExpensesPage o-- Checkbox : uses in forms indirectly
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ssjablonski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've reviewed this pull request which focuses on implementing the bill splitting functionality for trips on the frontend. The changes introduce a dedicated page for managing trip expenses and settlements, along with a summary dashboard on the main trip page. This involves adding new components for displaying expenses, balances, and settlements, as well as dialogs and forms for adding new expenses and confirming settlements, complete with client-side validation.
Highlights
- Dedicated Bill Splitting Page: A new page (
/trips/[id]/bill-splitting) has been added to provide a comprehensive view of trip finances, including expense history, user balances, and settlements. - Expense Management: Users can now add new expenses with various splitting methods (equal, percentage, amount, share) via a dedicated dialog form with validation.
- Balance and Settlement Views: Tabs are implemented on the bill splitting page to display current user balances within the group and suggested settlements to minimize debt.
- Settlement Confirmation: A dialog is available to confirm and record settlements between users, updating the group's financial state.
- Trip Expenses Dashboard: A summary component (
ExpensesDashboard) is added to the main trip page (/trips/[id]) to show a quick overview of user balances and provide a link to the full bill splitting page. - Form Validation: Client-side validation using Zod and Formik has been implemented for both the add expense and add settlement forms, including specific logic for different split methods.
Changelog
Click here to see the changelog
- frontend/app/(root)/trips/[id]/bill-splitting/page.tsx
- Added new page component for trip bill splitting.
- Implemented state management for error, balance, expenses, settlements, loading, dialog visibility, and active tab (lines 20-29).
- Added
useEffecthook to fetch data when the trip object is available (lines 33-36). - Implemented
fetchDatafunction to fetch group balance, expenses, minimum settlements, and settlement history concurrently (lines 38-70). - Implemented
handleExpenseSubmitfunction to handle adding new expenses, mapping split methods and preparing the payload for the API (lines 72-128). - Implemented
handleSettlementSubmitfunction to handle adding new settlements and preparing the payload for the API (lines 130-160). - Calculated paginated expenses based on search params and
ITEMS_PER_PAGE(lines 164-168). - Defined initial values structure for the add expense form (lines 170-180).
- Rendered loading state and error alerts (lines 182-192).
- Implemented tab navigation for 'Expense history', 'Users balances', and 'Settlements' (lines 194-228).
- Conditionally rendered
ExpensesHistoryTab,BalancesTab, andSettlementsTabbased on the active tab state (lines 230-255).
- frontend/app/(root)/trips/[id]/page.tsx
- Imported
ExpensesDashboardcomponent (line 23). - Added
<ExpensesDashboard trip={trip} />component to the main trip page (line 118).
- Imported
- frontend/components/bill-splitting/AddExpenseDialog.tsx
- Added new component for the 'Add Group Expense' dialog.
- Implemented Formik form with fields for description, total amount, and split method (lines 42-127).
- Used
FieldArrayto handle dynamic user shares based on the split method (lines 128-230). - Integrated Zod validation using
zod-formik-adapterwithexpenseFormSchema(line 45). - Displayed validation error messages using
ErrorMessage(lines 65-72, 93-100, 118-125, 210-217, 222-227). - Handled input types and steps based on the selected split method (lines 174-186).
- Included a submit button that is disabled while submitting (lines 231-233).
- frontend/components/bill-splitting/AddSettlementDialog.tsx
- Added new component for the 'Confirm settlement' dialog.
- Implemented Formik form for confirming a settlement amount (lines 48-112).
- Integrated Zod validation using
zod-formik-adapterwithsettlementFormSchema(lines 51-53). - Displayed validation error messages for the amount field (lines 89-96).
- Included cancel and confirm buttons (lines 98-108).
- Handled form submission to call the provided
onSubmitfunction (lines 61-70).
- frontend/components/bill-splitting/ExpensesDashboard.tsx
- Added new component to display a summary of group balances on the main trip page.
- Fetched group balances using
axios.get(lines 16-33). - Displayed user balances with different text colors based on positive, negative, or zero balance (lines 57-91).
- Included a button to navigate to the full bill splitting page (lines 93-99).
- frontend/components/bill-splitting/ExpensesHistory.tsx
- Added new component to display a paginated list of trip expenses.
- Mapped and displayed expense details including description, payer, total amount, and individual user shares (lines 13-50).
- Included date formatting for expense creation date (lines 23-25).
- Rendered
CustomPaginationcomponent (lines 53-57). - Displayed a message when there are no expenses (lines 60-62).
- frontend/components/bill-splitting/tabs/BalancesTab.tsx
- Added new component for the 'Users balances' tab.
- Displayed a list of user balances using the provided
balanceMap(lines 11-38). - Linked user emails to their account pages (lines 25-28).
- Styled balance amounts based on positive, negative, or zero values (lines 14-19, 31-34).
- frontend/components/bill-splitting/tabs/ExpensesHistoryTab.tsx
- Added new component for the 'Expense history' tab.
- Included the 'Add Expense' button which triggers the
AddExpenseDialog(lines 18-24). - Rendered the
ExpensesHistorycomponent to display the list of expenses (lines 26-31).
- frontend/components/bill-splitting/tabs/SettlementsTab.tsx
- Added new component for the 'Settlements' tab.
- Implemented state management for the settlement dialog (lines 12-17).
- Added
handleOpenSettlementfunction to set dialog state and selected user/amount (lines 19-28). - Implemented
findSettlementfunction to determine who owes whom and the amount (lines 30-38). - Displayed a list of users and their settlement status (you owe, they owe, or no settlement) (lines 46-94).
- Made settlement items clickable to open the confirmation dialog (lines 75-84).
- Displayed settlement history, showing who paid whom and the amount/date (lines 97-131).
- Rendered the
AddSettlementDialogcomponent, passing relevant data and the submit handler (lines 134-148).
- frontend/components/ui/checkbox.tsx
- Added a new reusable Checkbox component using Radix UI primitives.
- frontend/package-lock.json
- Added
@radix-ui/react-checkboxdependency. - Updated
@radix-ui/react-dialogdependency. - Updated various nested
@radix-uidependencies (@radix-ui/react-dialog,@radix-ui/react-slot,@radix-ui/react-dismissable-layer,@radix-ui/react-focus-scope,@radix-ui/react-portal,@radix-ui/react-primitive,@radix-ui/react-menu,@radix-ui/react-tooltip).
- Added
- frontend/package.json
- Added
@radix-ui/react-checkboxdependency (line 16). - Updated
@radix-ui/react-dialogdependency (line 17). - Removed
@radix-ui/react-slotfrom top-level dependencies (line 20).
- Added
- frontend/validation/expenseFormSchema.ts
- Added new file defining Zod schema for expense form validation.
- Defined
splitSchemaenum for split methods (line 3). - Defined
expenseFormSchemaobject with validation for description, total amount, split method, and shares array (lines 5-18). - Implemented
superRefinefor complex validation logic based on split method (lines 20-84). - Added specific validation rules for percentage (sum must be 100), amount (sum must equal total amount), and share (min 2 users/shares, positive integers) split methods (lines 23-83).
- frontend/validation/settlementFormSchema.ts
- Added new file defining Zod schema for settlement form validation.
- Defined
settlementFormSchemafunction that takesmaxAmountfor validation (lines 3-15). - Added validation for the amount field, ensuring it's positive and does not exceed
maxAmount(lines 5-14). - Exported
SettlementFormValuestype (lines 17-19).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Code Coverage
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive bill-splitting feature, which is a significant and valuable addition to the application. The implementation covers expense tracking, balance calculations, and settlements, along with the necessary UI components and validation.
The overall structure is good, leveraging Formik and Zod for forms and validation, which is a solid choice. The code is generally well-organized into components and tabs.
The main area for improvement across multiple new files is the consistent use of strong typing. Many props, state variables, and function parameters are currently untyped (using unknown or implicit any), which can reduce maintainability and type safety. Addressing these typing issues will greatly enhance the robustness and clarity of the new codebase.
Additionally, there are opportunities to refine error handling and abstract some repeated logic (like token checking).
No specific style guide was provided, so feedback is based on general TypeScript/React best practices, focusing on clarity, maintainability, and correctness.
Summary of Findings
- Type Safety: Numerous components and functions use
unknownor implicitanyfor props, state, and parameters (e.g.,GroupExpensesPagestate, props forAddExpenseDialog,ExpensesDashboard,BalancesTab,ExpensesHistoryTab,SettlementsTab). This is a high-severity concern impacting maintainability and correctness. Specific types should be defined and used. - Error Handling: Generic error messages like "An error occurred" are used in API handlers (e.g.,
GroupExpensesPage,ExpensesDashboard). More specific error handling or detailed logging in development would be beneficial. This is a medium-severity issue. - Code Duplication: Token checking logic is repeated across several API call handlers in
GroupExpensesPageandExpensesDashboard. Abstracting this would improve maintainability. This is a medium-severity issue. - Commented-Out Code: A large block of commented-out, seemingly duplicate code exists in
frontend/components/bill-splitting/ExpensesHistory.tsx. This should be removed. This is a medium-severity issue. - Type Coercion in Comparison: In
frontend/components/bill-splitting/ExpensesHistory.tsx,u.userId == uiduses==whereuidis a string andu.userIdis likely a number. Prefer===with explicit type conversion for clarity and safety. This is a medium-severity issue. - Untyped Mapped Items: Various
.map()and.find()callbacks operate on untyped items (e.g.,bingroupBalance.map,uintrip.memberships.mapacross multiple files). Stronger typing of the source arrays/objects would resolve this. This is a medium-severity issue.
Merge Readiness
This pull request adds significant new functionality for bill splitting. The core logic and component structure are well on their way. However, due to the widespread need for improved TypeScript typings (which is a high-severity concern for maintainability and preventing runtime errors) and other medium-severity issues like error handling and code duplication, I recommend that these changes be addressed before merging. Strengthening the type safety will make the new codebase much more robust and easier to maintain in the long run. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from team members after addressing the feedback.
| }: { | ||
| open: boolean; | ||
| setOpen: (v: boolean) => void; | ||
| trip: unknown; | ||
| onSubmit: (values: unknown, helpers: unknown) => Promise<void>; | ||
| initialValues: unknown; | ||
| }) { |
There was a problem hiding this comment.
The props trip, onSubmit values, and initialValues are typed as unknown. This significantly reduces type safety. Could these be given more specific types?
For example:
import { FormikHelpers } from 'formik';
import { Group } from '@/validation/groupSchema'; // Assuming Group type
import { z } from 'zod';
import { expenseFormSchema } from '@/validation/expenseFormSchema';
type ExpenseFormValues = z.infer<typeof expenseFormSchema>;
interface AddExpenseDialogProps {
open: boolean;
setOpen: (v: boolean) => void;
trip: Group; // Or a relevant subset of Group
onSubmit: (values: ExpenseFormValues, helpers: FormikHelpers<ExpenseFormValues>) => Promise<void>;
initialValues: ExpenseFormValues;
}
export default function AddExpenseDialog({
open,
setOpen,
trip,
onSubmit,
initialValues,
}: AddExpenseDialogProps) { /* ... */ }| export default function SettlementsTab({ | ||
| settlements, | ||
| trip, | ||
| currentUserId, | ||
| history, | ||
| onSubmit, | ||
| }: unknown) { |
There was a problem hiding this comment.
Similar to other components, all props for SettlementsTab are effectively any due to the props object being typed as unknown. This should be replaced with a specific props interface.
Example structure:
interface SettlementItem { /* ... */ }
interface TripType { /* ... */ }
interface SettlementHistoryItem { /* ... */ }
interface SettlementFormSubmitValues { /* ... */ }
interface SettlementsTabProps {
settlements: SettlementItem[];
trip: TripType;
currentUserId: number;
history: SettlementHistoryItem[];
onSubmit: (values: SettlementFormSubmitValues) => Promise<void>;
}
export default function SettlementsTab({
// ...destructure props
}: SettlementsTabProps) {
// ...
}Defining these types will greatly improve code quality.
| export default function ExpensesHistoryTab({ | ||
| showDialog, | ||
| setShowDialog, | ||
| trip, | ||
| initialValues, | ||
| onSubmit, | ||
| paginatedExpenses, | ||
| expenses, | ||
| ITEMS_PER_PAGE, | ||
| }: unknown) { |
There was a problem hiding this comment.
All props for ExpensesHistoryTab are effectively any because the entire props object is typed as unknown. This undermines type safety. Please define an interface for these props with specific types for each.
Example structure:
interface ExpensesHistoryTabProps {
showDialog: boolean;
setShowDialog: (open: boolean) => void;
trip: YourTripType; // Specific type for trip
initialValues: YourExpenseFormInitialValuesType; // Specific type
onSubmit: (values: YourExpenseFormValuesType) => Promise<void>; // Specific type
paginatedExpenses: YourExpenseItemType[]; // Specific type
expenses: YourExpenseItemType[]; // Specific type
ITEMS_PER_PAGE: number;
}
export default function ExpensesHistoryTab({
// ...destructure props
}: ExpensesHistoryTabProps) {
// ...
}| import Link from 'next/link'; | ||
| import { Banknote } from 'lucide-react'; | ||
|
|
||
| export default function BalancesTab({ trip, balanceMap }: unknown) { |
There was a problem hiding this comment.
The props trip and balanceMap are part of an unknown object. It's recommended to define the props object explicitly with types.
Consider:
interface BalancesTabProps {
trip: YourTripType; // Replace YourTripType with the actual type
balanceMap: Map<number, number>; // Assuming userId (number) to balance (number)
}
export default function BalancesTab({ trip, balanceMap }: BalancesTabProps) {
// ...
}What are the specific types for trip and the keys/values in balanceMap?
export default function BalancesTab({ trip, balanceMap }: { trip: any; balanceMap: Map<number, number> }) { // TODO: Replace 'any' with a specific Trip type| import { Button } from '@/components/ui/button'; | ||
| import { useRouter } from 'next/navigation'; | ||
|
|
||
| export default function ExpansesDashboard({ trip }: unknown) { |
There was a problem hiding this comment.
The trip prop is part of an unknown object ({ trip }: unknown). It's better to define the props object explicitly with types.
Consider changing to:
interface ExpansesDashboardProps {
trip: YourTripType; // Replace YourTripType with the actual type of trip
}
export default function ExpansesDashboard({ trip }: ExpansesDashboardProps) {
// ...
}What is the expected type for the trip object?
export default function ExpansesDashboard({ trip }: { trip: any }) { // TODO: Replace 'any' with a specific Trip type| } | ||
| }; | ||
|
|
||
| const handleExpenseSubmit = async (values) => { |
There was a problem hiding this comment.
The values parameter for handleExpenseSubmit is implicitly any. It would be beneficial to define an interface or type for these form values, ideally derived from your Zod schema (expenseFormSchema), to ensure type safety and clarity.
For example, using z.infer:
import { z } from 'zod';
import { expenseFormSchema } from '@/validation/expenseFormSchema'; // Adjust path
type ExpenseFormValues = z.infer<typeof expenseFormSchema>;
const handleExpenseSubmit = async (values: ExpenseFormValues) => {
// ...
};This would also apply to handleSettlementSubmit.
| const fetchData = async () => { | ||
| try { | ||
| const token = getToken(); | ||
| if (!token) { | ||
| logout(); | ||
| return; | ||
| } | ||
| const [balanceRes, expensesRes, settlementsRes, settlementsHistoryRes] = | ||
| await Promise.all([ | ||
| axios.get(`/api/v1/groups/${trip.id}/balance/balances`, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| axios.get(`/api/v1/groups/${trip.id}/expenses`, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| axios.get(`/api/v1/groups/${trip.id}/balance/min`, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| axios.get(`/api/v1/groups/${trip.id}/settlements`, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| ]); | ||
|
|
||
| setGroupBalance(balanceRes.data); | ||
| setExpenses(expensesRes.data.content); | ||
| setSettlements(settlementsRes.data); | ||
| setSettlementsHistory(settlementsHistoryRes.data); | ||
| } catch (err) { | ||
| setError(err.message || 'An error occurred'); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The token checking logic (getToken(); if (!token) { logout(); return; }) is repeated in fetchData, handleExpenseSubmit, and handleSettlementSubmit. Could this be abstracted, perhaps into a higher-order function that wraps API calls needing authentication, or handled more centrally by axiosInstance if appropriate for all requests?
Also, while err.message || 'An error occurred' provides a fallback, for better debugging and user feedback, could more specific error messages be set based on the error type or status code, or could the full error be logged in development environments?
| onSubmit, | ||
| }: unknown) { | ||
| const [showDialog, setShowDialog] = useState(false); | ||
| const [selectedUser, setSelectedUser] = useState<unknown>(null); |
There was a problem hiding this comment.
The state selectedUser is typed as unknown. It seems to store a user ID. Would number | null be a more appropriate type?
const [selectedUser, setSelectedUser] = useState<number | null>(null);| const youOwe = settlements.find( | ||
| (s: unknown) => s.debtorId === currentUserId && s.creditorId === userId, | ||
| ); | ||
| const theyOwe = settlements.find( | ||
| (s: unknown) => s.debtorId === userId && s.creditorId === currentUserId, | ||
| ); |
There was a problem hiding this comment.
The item s in settlements.find((s: unknown) => ...) is typed as unknown. If the settlements prop is strongly typed (e.g., SettlementItem[]), s would infer its type. What is the expected structure of a settlement item?
| .filter((user: unknown) => user.userId !== currentUserId) | ||
| .map((user: unknown) => { |
There was a problem hiding this comment.
The user parameter in trip?.memberships.filter(...).map((user: unknown) => ...) is typed as unknown. If the trip prop is strongly typed, user would infer its type. What is the type for a membership item?
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ssjablonski - I've reviewed your changes - here's some feedback:
Blocking issues:
- It looks like no matter how values.splitMethod === 'share' is evaluated, this expression returns 'number'. This is probably a copy-paste error. (link)
General comments:
- Fix the typo in the ExpensesDashboard component name ('ExpansesDashboard') so that the file name, export, and imports all match.
- Extract the repeated getToken()/Authorization header logic into a shared axios instance or helper to DRY up your API calls.
- Replace the many
unknownprop types with explicit TypeScript interfaces for better type safety and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| <ExpensesDashboard trip={trip} /> | ||
| <button | ||
| onClick={() => redirect(`/trips/${trip.id}/activities/create`)} |
There was a problem hiding this comment.
issue (bug_risk): Server-side redirect used in client event handler
Use useRouter().push(...) or a <Link> for client-side navigation instead of redirect, which only works server-side.
| import { Button } from '@/components/ui/button'; | ||
| import { useRouter } from 'next/navigation'; | ||
|
|
||
| export default function ExpansesDashboard({ trip }: unknown) { |
There was a problem hiding this comment.
issue (typo): Component name typo mismatches file name
Rename the function to ExpensesDashboard to match the file and imports, preventing confusion and potential import errors.
| export default function ExpansesDashboard({ trip }: unknown) { | |
| export default function ExpensesDashboard({ trip }: unknown) { |
| {({ values, errors, touched, isSubmitting, setFieldValue }) => ( | ||
| <Form className="space-y-4"> | ||
| <div className="mt-4"> | ||
| <label className="font-semibold" htmlFor="name"> |
There was a problem hiding this comment.
issue: htmlFor attribute doesn’t match the input field
Update the htmlFor value to match the input's id, or set id="description" on the Field to ensure correct label association for accessibility.
| <Field | ||
| type="checkbox" | ||
| name={`shares.${idx}.included`} | ||
| checked={user.included} | ||
| onChange={( | ||
| e: React.ChangeEvent<HTMLInputElement>, | ||
| ) => | ||
| setFieldValue( | ||
| `shares.${idx}.included`, | ||
| e.target.checked, | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
suggestion: Use the custom Checkbox component instead of a native input
Integrate the Checkbox component here to maintain consistent styling and accessibility.
| <Field | |
| type="checkbox" | |
| name={`shares.${idx}.included`} | |
| checked={user.included} | |
| onChange={( | |
| e: React.ChangeEvent<HTMLInputElement>, | |
| ) => | |
| setFieldValue( | |
| `shares.${idx}.included`, | |
| e.target.checked, | |
| ) | |
| } | |
| /> | |
| <Checkbox | |
| name={`shares.${idx}.included`} | |
| checked={user.included} | |
| onCheckedChange={(checked: boolean) => | |
| setFieldValue( | |
| `shares.${idx}.included`, | |
| checked, | |
| ) | |
| } | |
| aria-label={`Include ${user.name || 'user'}`} | |
| /> |
| // } | ||
| // return errors; | ||
| // }} | ||
| onSubmit={(values, { setSubmitting }) => { |
There was a problem hiding this comment.
suggestion (bug_risk): Formik onSubmit handler doesn’t await the async call
If you close the dialog before awaiting the async operation, users may not see API errors. Make the handler async, await the onSubmit call, then call setSubmitting(false) and close the dialog.
Suggested implementation:
+ onSubmit={async (values, { setSubmitting }) => {
+ try {
+ await onSubmit({
+ groupId,+ onSubmit({
+ groupId,+ });
+ } finally {
+ setSubmitting(false);
+ onOpenChange(false);
+ }
+ }}| } catch (err) { | ||
| setError(err.message || 'An error occurred'); | ||
| } finally { |
There was a problem hiding this comment.
suggestion (bug_risk): Accessing err.message on unknown error type
Narrow the type of err before accessing message, for example with if (err instanceof Error) or axios.isAxiosError to ensure type safety.
| } catch (err) { | |
| setError(err.message || 'An error occurred'); | |
| } finally { | |
| } catch (err) { | |
| if (err instanceof Error) { | |
| setError(err.message); | |
| } else { | |
| setError('An error occurred'); | |
| } | |
| } finally { |
| values.splitMethod === 'share' | ||
| ? 'number' | ||
| : 'number' |
There was a problem hiding this comment.
security (useless-ternary): It looks like no matter how values.splitMethod === 'share' is evaluated, this expression returns 'number'. This is probably a copy-paste error.
Source: opengrep
| const user = appStore((state) => state.user); | ||
|
|
||
| useEffect(() => { | ||
| if (!trip || !trip.id) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!trip || !trip.id) return; | |
| if (!trip || !trip.id) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Code Coverage
|
Code Coverage
|
1 similar comment
Code Coverage
|
Code Coverage
|
Code Coverage
|
Code Coverage
|
Code Coverage
|
1 participant
Summary by Sourcery
Add a complete bill-splitting workflow to trip pages, including an expenses dashboard on the main trip view and a dedicated multi-tab interface for expense history, balances, and settlements with forms and validation.
New Features:
/trips/[id]/bill-splittingwith tabs for expense history, user balances, and settlementsEnhancements:
@radix-ui/react/dialogdependencyCheckboxUI component and Zod-based validation schemas for expense and settlement formsBuild:
package.jsonto adjust Radix UI dependencies and remove unused packages