Fixes #2, Fixes #3, Fixes #5, Fixes #20: unify activity names, currency formatting, and web auth - #22
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates currency formatting into a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend
participant DB as Prisma
Client->>Backend: GET /activity (groupId/userId, cursor)
Backend->>DB: prisma.activity.findMany(include: { actor, group })
DB-->>Backend: activities (actor/group may include name)
Backend->>DB: prisma.user.findMany(ids from actor/payer/receiver)
DB-->>Backend: users { id, name }
Backend->>Backend: mapActivities -> enrichMetadata (attach actorName/groupName/payerName/receiverName)
Backend-->>Client: Paginated ActivityDto items with enriched name fields and ISO timestamps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/groups/SettlementList.tsx (1)
10-13:⚠️ Potential issue | 🟡 MinorRemove unsafe
CurrencyCodecast by properly typing thecurrencyprop.The
currencyprop is typed asstringbut receivesCurrencyCodevalues from the call site and is force-cast toCurrencyCodeat line 42. This masks a type mismatch and weakens type safety. The prop should be typed asCurrencyCodedirectly.Suggested refactor
type SettlementListProps = { groupId: string; - currency: string; + currency: CurrencyCode; suggestions: SimplifySuggestionDto[]; memberLookup: Record<string, { name: string; email: string }>; initialReminderActivity: ActivityDto[]; }; -const formatAmount = (cents: string) => formatCurrencyFromCents(cents, currency as CurrencyCode); +const formatAmount = (cents: string) => formatCurrencyFromCents(cents, currency);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/groups/SettlementList.tsx` around lines 10 - 13, The prop type for the SettlementList component is declared as string but callers pass CurrencyCode values and the code force-casts currency to CurrencyCode (unsafe); update the SettlementListProps type so currency is typed as CurrencyCode instead of string, remove the manual cast where currency is coalesced or used (the forced cast currently near the usage of currency in the component), and ensure any callers still provide a CurrencyCode-typed value so the type system enforces correctness (refer to SettlementListProps and the component's currency usage to locate changes).
🧹 Nitpick comments (5)
README.md (1)
127-128: Clarify scope oflocalStorageusage to avoid ambiguity.At Line 128, the statement is correct for auth, but readers may interpret it as “no
localStorageanywhere in web.” Consider clarifying that this applies only to authentication tokens, while non-auth preferences (e.g., theme) may still uselocalStorage.Proposed wording tweak
- - `localStorage` is not part of the web auth flow + - `localStorage` is not used for web authentication tokens (it may still be used for non-auth preferences like theme)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 127 - 128, Clarify that the statement refers only to authentication tokens: update the README sentence that mentions auth tokens and localStorage to explicitly say that authentication tokens are stored exclusively in httpOnly cookies (handled by apps/web/app/api/auth/*, apps/web/src/lib/backend.ts, and apps/web/middleware.ts), while non-auth client-side data such as UI preferences (e.g., theme) may still be stored in localStorage. Keep the mention of the three auth files and add a short parenthetical or sentence distinguishing "auth tokens" from other uses of localStorage.apps/mobile/app/screens/GroupDetailScreen.tsx (2)
120-120: Unnecessary cast:GroupDto.currencyis already typed asCurrencyCode.Per the type definition,
GroupDto.currencyisCurrencyCode, so the cast is redundant. The?? 'USD'fallback handles the case whengroupis null.Minor cleanup
- const groupCurrency = (group?.currency ?? 'USD') as CurrencyCode; + const groupCurrency = group?.currency ?? 'USD';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/screens/GroupDetailScreen.tsx` at line 120, Remove the redundant type cast on groupCurrency: the expression const groupCurrency = (group?.currency ?? 'USD') as CurrencyCode; should be simplified because GroupDto.currency is already CurrencyCode; update the code to use const groupCurrency = group?.currency ?? 'USD' (or an equivalent without the as CurrencyCode) and ensure any places expecting CurrencyCode still accept the result.
405-405: Same float round-trip issue: prefer using cents string directly.
userBalanceis derived fromNumber(perUserOwedCents[userId]) / 100, then multiplied back by 100 here. This introduces unnecessary floating-point risk.Consider computing the absolute cents value directly from
summary.perUserOwedCents[currentUserId]:Proposed approach
// Compute absolute cents as string once const userOwedCentsStr = summary?.perUserOwedCents[currentUserId ?? ''] ?? '0'; const userOwedCentsBigInt = BigInt(userOwedCentsStr); const absUserOwedCents = (userOwedCentsBigInt < 0n ? -userOwedCentsBigInt : userOwedCentsBigInt).toString(); // Then use in both places: formatCurrencyFromCents(absUserOwedCents, groupCurrency)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/screens/GroupDetailScreen.tsx` at line 405, The current code computes display cents by converting summary.perUserOwedCents to a Number (userBalance) and then multiplying by 100 which can reintroduce float errors; instead read the raw cents string from summary.perUserOwedCents[currentUserId], convert to a BigInt (or absolute-string) to get an absolute cents value, and pass that absolute cents string directly to formatCurrencyFromCents; update the Text render (where userBalance and formatCurrencyFromCents are used) to use this precomputed absCents string and keep the existing color logic based on userBalance but avoid the Math.round/ *100 round-trip.apps/mobile/app/screens/HomeScreen.tsx (1)
191-191: Simplify: UsetotalBalanceCentsstring directly instead of round-tripping through float.The current approach converts cents string → number → divide by 100 → multiply by 100 → round. This is error-prone and unnecessary since
summary.totalBalanceCentsis already in cents.Proposed fix
- amount={formatCurrencyFromCents(Math.round(Math.abs(totalBalance) * 100), 'USD')} + amount={formatCurrencyFromCents( + BigInt(summary?.totalBalanceCents ?? '0') < 0n + ? (-BigInt(summary?.totalBalanceCents ?? '0')).toString() + : (summary?.totalBalanceCents ?? '0'), + 'USD' + )}Or extract to a helper for clarity:
const absBalanceCents = summary?.totalBalanceCents ? (BigInt(summary.totalBalanceCents) < 0n ? -BigInt(summary.totalBalanceCents) : BigInt(summary.totalBalanceCents)).toString() : '0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/screens/HomeScreen.tsx` at line 191, The amount calculation is converting summary.totalBalanceCents from string → number → float and back which is unnecessary and error-prone; replace the Math.round(Math.abs(totalBalance) * 100) chain by deriving an absolute cents string directly from summary.totalBalanceCents and passing that into formatCurrencyFromCents. Locate the usage in HomeScreen.tsx where amount={formatCurrencyFromCents(Math.round(Math.abs(totalBalance) * 100), 'USD')} and replace it with a value computed from summary?.totalBalanceCents (e.g., compute absBalanceCents by parsing the cents string with BigInt or string sign-check and taking absolute, defaulting to "0") then call formatCurrencyFromCents(absBalanceCents, 'USD'). Ensure the helper or inline logic handles null/undefined summary and negative values.apps/backend/src/activity/activity.service.ts (1)
51-51: Potential issue:user.namecan benull, which affects downstream logic.When
user.nameisnullin the database,userNamesById[userId]will benull(notundefined). This causes:
- Line 60:
userNamesById[event.actorUserId] ?? event.actor?.namewon't fall back becausenullis not nullish for??- Lines 16-17: The truthiness check
userNamesById[payerId]will correctly skipnull, but if you later change to!= null, it could addpayerName: nullConsider filtering out null names:
Proposed fix
- const userNamesById = Object.fromEntries(users.map((user) => [user.id, user.name])); + const userNamesById: Record<string, string> = Object.fromEntries( + users.filter((user) => user.name != null).map((user) => [user.id, user.name as string]) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/activity/activity.service.ts` at line 51, The userNamesById map currently stores null values from users.map which prevents the `??` fallback and can propagate nulls; update the map creation in activity.service (the userNamesById construction) to exclude or normalize null names—e.g. build it with Object.fromEntries(users.map(u => [u.id, u.name ?? undefined]).filter(([,name]) => name !== undefined)) so lookups like userNamesById[event.actorUserId] ?? event.actor?.name and checks around payerId/payerName behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/activity/activity.service.ts`:
- Line 81: Replace the unsafe "as never" cast on the activity creation payload:
remove "type: params.type as never" and instead use a proper cast that preserves
type safety (e.g., "type: params.type as Prisma.ActivityType") or ensure the
shared-types union is declared "as const" so no cast is needed; locate the usage
of params.type in activity.service.ts (the ActivityType parameter around line
73) and update that assignment accordingly.
In `@apps/web/src/components/groups/RecurringExpenseList.tsx`:
- Line 280: The prop typing for RecurringExpenseList is too permissive: update
the RecurringExpenseListProps to type the currency prop as CurrencyCode (the
union used by formatCurrencyFromCents) instead of string, then remove the unsafe
cast (currency as CurrencyCode) where
formatCurrencyFromCents(item.totalAmountCents, currency as CurrencyCode) is
called; ensure any call sites pass a valid CurrencyCode or are adjusted
accordingly so TypeScript enforces the allowed currency values at compile time.
In `@packages/shared-types/src/index.ts`:
- Around line 8-35: normalizeCentsInput currently accepts any trimmed string and
lets invalid values like "12.3" or "abc" through; update normalizeCentsInput to
validate string inputs so they match an optional leading '-' followed by only
digits (regex like /^-?\d+$/) and throw a clear Error for anything else,
ensuring formatCurrencyFromCents receives only integer-cent strings; keep
bigint/number handling the same and reference normalizeCentsInput and
formatCurrencyFromCents (and existing CURRENCY_SYMBOLS/CurrencyCode usages) when
making the change.
---
Outside diff comments:
In `@apps/web/src/components/groups/SettlementList.tsx`:
- Around line 10-13: The prop type for the SettlementList component is declared
as string but callers pass CurrencyCode values and the code force-casts currency
to CurrencyCode (unsafe); update the SettlementListProps type so currency is
typed as CurrencyCode instead of string, remove the manual cast where currency
is coalesced or used (the forced cast currently near the usage of currency in
the component), and ensure any callers still provide a CurrencyCode-typed value
so the type system enforces correctness (refer to SettlementListProps and the
component's currency usage to locate changes).
---
Nitpick comments:
In `@apps/backend/src/activity/activity.service.ts`:
- Line 51: The userNamesById map currently stores null values from users.map
which prevents the `??` fallback and can propagate nulls; update the map
creation in activity.service (the userNamesById construction) to exclude or
normalize null names—e.g. build it with Object.fromEntries(users.map(u => [u.id,
u.name ?? undefined]).filter(([,name]) => name !== undefined)) so lookups like
userNamesById[event.actorUserId] ?? event.actor?.name and checks around
payerId/payerName behave correctly.
In `@apps/mobile/app/screens/GroupDetailScreen.tsx`:
- Line 120: Remove the redundant type cast on groupCurrency: the expression
const groupCurrency = (group?.currency ?? 'USD') as CurrencyCode; should be
simplified because GroupDto.currency is already CurrencyCode; update the code to
use const groupCurrency = group?.currency ?? 'USD' (or an equivalent without the
as CurrencyCode) and ensure any places expecting CurrencyCode still accept the
result.
- Line 405: The current code computes display cents by converting
summary.perUserOwedCents to a Number (userBalance) and then multiplying by 100
which can reintroduce float errors; instead read the raw cents string from
summary.perUserOwedCents[currentUserId], convert to a BigInt (or
absolute-string) to get an absolute cents value, and pass that absolute cents
string directly to formatCurrencyFromCents; update the Text render (where
userBalance and formatCurrencyFromCents are used) to use this precomputed
absCents string and keep the existing color logic based on userBalance but avoid
the Math.round/ *100 round-trip.
In `@apps/mobile/app/screens/HomeScreen.tsx`:
- Line 191: The amount calculation is converting summary.totalBalanceCents from
string → number → float and back which is unnecessary and error-prone; replace
the Math.round(Math.abs(totalBalance) * 100) chain by deriving an absolute cents
string directly from summary.totalBalanceCents and passing that into
formatCurrencyFromCents. Locate the usage in HomeScreen.tsx where
amount={formatCurrencyFromCents(Math.round(Math.abs(totalBalance) * 100),
'USD')} and replace it with a value computed from summary?.totalBalanceCents
(e.g., compute absBalanceCents by parsing the cents string with BigInt or string
sign-check and taking absolute, defaulting to "0") then call
formatCurrencyFromCents(absBalanceCents, 'USD'). Ensure the helper or inline
logic handles null/undefined summary and negative values.
In `@README.md`:
- Around line 127-128: Clarify that the statement refers only to authentication
tokens: update the README sentence that mentions auth tokens and localStorage to
explicitly say that authentication tokens are stored exclusively in httpOnly
cookies (handled by apps/web/app/api/auth/*, apps/web/src/lib/backend.ts, and
apps/web/middleware.ts), while non-auth client-side data such as UI preferences
(e.g., theme) may still be stored in localStorage. Keep the mention of the three
auth files and add a short parenthetical or sentence distinguishing "auth
tokens" from other uses of localStorage.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 063b3440-1219-417c-8b93-a0f632bf4600
📒 Files selected for processing (26)
README.mdapps/backend/src/activity/activity.service.spec.tsapps/backend/src/activity/activity.service.tsapps/backend/src/common/utils/money.util.tsapps/mobile/app/components/ui/MoneyText.tsxapps/mobile/app/screens/ActivityScreen.tsxapps/mobile/app/screens/AddExpenseScreen.tsxapps/mobile/app/screens/ExpenseDetailScreen.tsxapps/mobile/app/screens/GroupDetailScreen.tsxapps/mobile/app/screens/GuestGroupDetailScreen.tsxapps/mobile/app/screens/HomeScreen.tsxapps/mobile/app/services/group.service.tsapps/web/app/dashboard/activity/page.tsxapps/web/app/dashboard/groups/[groupId]/page.tsxapps/web/app/dashboard/page.tsxapps/web/app/share/[token]/page.tsxapps/web/lib/api.tsapps/web/src/components/activity/ActivityFeed.tsxapps/web/src/components/dashboard/ActivityList.tsxapps/web/src/components/groups/ExpenseDetailCard.tsxapps/web/src/components/groups/ExpenseRow.tsxapps/web/src/components/groups/ExpenseTable.tsxapps/web/src/components/groups/GroupSummaryPanel.tsxapps/web/src/components/groups/RecurringExpenseList.tsxapps/web/src/components/groups/SettlementList.tsxpackages/shared-types/src/index.ts
💤 Files with no reviewable changes (1)
- apps/web/lib/api.ts
Contributor
|
Note Docstrings generation - SUCCESS |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 9 file(s) based on 3 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
Docstrings generation was requested by @Arun-kushwaha007. The following files were modified: * `apps/mobile/app/components/ui/MoneyText.tsx` * `apps/mobile/app/screens/ActivityScreen.tsx` * `apps/mobile/app/screens/AddExpenseScreen.tsx` * `apps/mobile/app/screens/ExpenseDetailScreen.tsx` * `apps/mobile/app/screens/GroupDetailScreen.tsx` * `apps/mobile/app/screens/GuestGroupDetailScreen.tsx` * `apps/mobile/app/screens/HomeScreen.tsx` * `apps/web/app/dashboard/activity/page.tsx` * `apps/web/app/dashboard/groups/[groupId]/page.tsx` * `apps/web/app/dashboard/page.tsx` * `apps/web/app/share/[token]/page.tsx` * `apps/web/src/components/activity/ActivityFeed.tsx` * `apps/web/src/components/dashboard/ActivityList.tsx` * `apps/web/src/components/groups/ExpenseDetailCard.tsx` * `apps/web/src/components/groups/ExpenseRow.tsx` * `apps/web/src/components/groups/ExpenseTable.tsx` * `apps/web/src/components/groups/GroupSummaryPanel.tsx` * `apps/web/src/components/groups/RecurringExpenseList.tsx` * `apps/web/src/components/groups/SettlementList.tsx` * `packages/shared-types/src/index.ts` These files were ignored: * `apps/backend/src/activity/activity.service.spec.ts` These file types are not supported: * `README.md`
- Update activity type casting in backend service for better Prisma compatibility - Refine currency prop type in web component to prevent type mismatches - Add accessibility titles to select elements in recurring expense list - Enhance currency cents input normalization with integer validation in shared types Fixes #2, #3, #5, #20
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/mobile/app/screens/GuestGroupDetailScreen.tsx (1)
161-166: Consider removing redundant group name on single-group view.Since this is the
GuestGroupDetailScreenshowing activities for a specific shared group, appendingactivity.groupNameto each subtitle is redundant—all activities are already scoped to this group. Removing it would reduce visual noise.✨ Suggested simplification
<ActivityItem title={activity.type.replace(/_/g, ' ')} - subtitle={`Recorded by ${activity.actorName ?? 'a member'}${activity.groupName ? ` in ${activity.groupName}` : ''}`} + subtitle={`Recorded by ${activity.actorName ?? 'a member'}`} date={new Date(activity.createdAt).toLocaleDateString()} icon="history" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/screens/GuestGroupDetailScreen.tsx` around lines 161 - 166, In GuestGroupDetailScreen, the ActivityItem subtitle currently appends activity.groupName even though activities are scoped to the current group; remove the redundant suffix by changing the subtitle passed to ActivityItem (in the ActivityItem usage inside GuestGroupDetailScreen) to only include the actor text (e.g., `Recorded by ${activity.actorName ?? 'a member'}`) and stop referencing activity.groupName so the subtitle is simplified and less noisy.apps/web/src/components/activity/ActivityFeed.tsx (1)
250-252:formatAmount(item)is called twice per item.The function is invoked both in the condition and the span content. Extract to a variable to avoid redundant computation.
♻️ Proposed fix
</div> - {formatAmount(item) ? ( - <span className="text-xs sm:text-sm font-bold text-[var(--fs-text-primary)]">{formatAmount(item)}</span> - ) : null} + {(() => { + const amount = formatAmount(item); + return amount ? ( + <span className="text-xs sm:text-sm font-bold text-[var(--fs-text-primary)]">{amount}</span> + ) : null; + })()} </div>Alternatively, compute
amountalongsideaccentat the top of the map callback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/activity/ActivityFeed.tsx` around lines 250 - 252, The code calls formatAmount(item) twice inside the ActivityFeed map render; to avoid redundant computation, compute a const amount = formatAmount(item) (and optionally const accent if already computed) at the top of the map callback or component render for each item, then use {amount ? (<span ...>{amount}</span>) : null} instead of calling formatAmount again; update the JSX that references formatAmount(item) to use the new amount variable and remove the duplicate calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/activity/ActivityFeed.tsx`:
- Around line 50-55: The settlement_reminder branch constructs payer/receiver
labels by falling back to payerId.slice(0,8) and receiverId.slice(0,8), which
yields empty strings when IDs are ''. Update the logic in the
'settlement_reminder' case (variables payerId, receiverId, payer, receiver,
actor) so that after checking metadata.payerName/receiverName you fall back to a
non-empty truncated ID only if payerId/receiverId is truthy, otherwise use a
stable placeholder (e.g., "<unknown>" or "someone"); ensure the final returned
string uses these placeholders to avoid producing empty labels.
---
Nitpick comments:
In `@apps/mobile/app/screens/GuestGroupDetailScreen.tsx`:
- Around line 161-166: In GuestGroupDetailScreen, the ActivityItem subtitle
currently appends activity.groupName even though activities are scoped to the
current group; remove the redundant suffix by changing the subtitle passed to
ActivityItem (in the ActivityItem usage inside GuestGroupDetailScreen) to only
include the actor text (e.g., `Recorded by ${activity.actorName ?? 'a member'}`)
and stop referencing activity.groupName so the subtitle is simplified and less
noisy.
In `@apps/web/src/components/activity/ActivityFeed.tsx`:
- Around line 250-252: The code calls formatAmount(item) twice inside the
ActivityFeed map render; to avoid redundant computation, compute a const amount
= formatAmount(item) (and optionally const accent if already computed) at the
top of the map callback or component render for each item, then use {amount ?
(<span ...>{amount}</span>) : null} instead of calling formatAmount again;
update the JSX that references formatAmount(item) to use the new amount variable
and remove the duplicate calls.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad4fe493-7ef3-4020-a068-98bd3e166531
📒 Files selected for processing (20)
apps/mobile/app/components/ui/MoneyText.tsxapps/mobile/app/screens/ActivityScreen.tsxapps/mobile/app/screens/AddExpenseScreen.tsxapps/mobile/app/screens/ExpenseDetailScreen.tsxapps/mobile/app/screens/GroupDetailScreen.tsxapps/mobile/app/screens/GuestGroupDetailScreen.tsxapps/mobile/app/screens/HomeScreen.tsxapps/web/app/dashboard/activity/page.tsxapps/web/app/dashboard/groups/[groupId]/page.tsxapps/web/app/dashboard/page.tsxapps/web/app/share/[token]/page.tsxapps/web/src/components/activity/ActivityFeed.tsxapps/web/src/components/dashboard/ActivityList.tsxapps/web/src/components/groups/ExpenseDetailCard.tsxapps/web/src/components/groups/ExpenseRow.tsxapps/web/src/components/groups/ExpenseTable.tsxapps/web/src/components/groups/GroupSummaryPanel.tsxapps/web/src/components/groups/RecurringExpenseList.tsxapps/web/src/components/groups/SettlementList.tsxpackages/shared-types/src/index.ts
✅ Files skipped from review due to trivial changes (6)
- apps/web/src/components/groups/RecurringExpenseList.tsx
- apps/mobile/app/screens/ExpenseDetailScreen.tsx
- apps/web/src/components/dashboard/ActivityList.tsx
- apps/web/src/components/groups/ExpenseRow.tsx
- apps/mobile/app/screens/ActivityScreen.tsx
- apps/web/app/dashboard/page.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/mobile/app/screens/AddExpenseScreen.tsx
- apps/web/src/components/groups/SettlementList.tsx
- apps/mobile/app/components/ui/MoneyText.tsx
- apps/web/src/components/groups/GroupSummaryPanel.tsx
- packages/shared-types/src/index.ts
- apps/web/src/components/groups/ExpenseDetailCard.tsx
- apps/mobile/app/screens/GroupDetailScreen.tsx
- apps/mobile/app/screens/HomeScreen.tsx
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 7 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 7 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Fixed 9 file(s) based on 3 unresolved review comments. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: Arun Kushwaha <121769639+Arun-kushwaha007@users.noreply.github.com>
Docstrings generation was requested by @Arun-kushwaha007. The following files were modified: * `apps/mobile/app/components/ui/MoneyText.tsx` * `apps/mobile/app/screens/ActivityScreen.tsx` * `apps/mobile/app/screens/ExpenseDetailScreen.tsx` * `apps/mobile/app/screens/GuestGroupDetailScreen.tsx` * `apps/mobile/app/screens/HomeScreen.tsx` * `apps/web/app/dashboard/groups/[groupId]/page.tsx` * `apps/web/app/dashboard/page.tsx` * `apps/web/src/components/activity/ActivityFeed.tsx` * `apps/web/src/components/dashboard/ActivityList.tsx` * `apps/web/src/components/groups/GroupSummaryPanel.tsx` * `apps/web/src/components/groups/RecurringExpenseList.tsx` * `apps/web/src/components/groups/SettlementList.tsx` * `packages/shared-types/src/index.ts` These files were kept as they were: * `apps/mobile/app/screens/AddExpenseScreen.tsx` * `apps/mobile/app/screens/GroupDetailScreen.tsx` * `apps/web/app/dashboard/activity/page.tsx` * `apps/web/app/share/[token]/page.tsx` * `apps/web/src/components/groups/ExpenseDetailCard.tsx` * `apps/web/src/components/groups/ExpenseRow.tsx` * `apps/web/src/components/groups/ExpenseTable.tsx` These files were ignored: * `apps/backend/src/activity/activity.service.spec.ts` These file types are not supported: * `README.md` * `packages/shared-types/tsconfig.json` * `tsconfig.base.json`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/activity/ActivityFeed.tsx`:
- Around line 75-80: The current logic picks amountCents before checking type
and returns null if it's not a string, which blocks valid numeric values and
prevents fallback to totalAmountCents; change the logic in ActivityFeed (where
cents is computed from activity.metadata?.amountCents and
activity.metadata?.totalAmountCents) to allow numeric or string cents values and
remove the early return — compute cents as the first non-nullish of amountCents
and totalAmountCents, then always call formatCurrencyFromCents(cents,
currencyOrDefault) (formatCurrencyFromCents already accepts numbers), keeping
the same currency selection logic (currency === 'USD' || 'EUR' || 'INR' ?
currency : 'USD').
In `@tsconfig.base.json`:
- Around line 4-5: The change sets "moduleResolution": "Bundler" while keeping
"module": "CommonJS" in tsconfig.base.json which requires TypeScript 6+, but the
repo is pinned to 5.7.3; either revert the base config to a
TypeScript-5.7-compatible setting (e.g., restore "moduleResolution" to "Node" or
remove the Bundler value) or move "moduleResolution": "Bundler" into
per-workspace tsconfig overrides after you upgrade all packages to TS 6.0+;
target the keys "module" and "moduleResolution" in tsconfig.base.json and ensure
consistency across workspace tsconfigs or perform a coordinated TypeScript
upgrade before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69d687b2-7ccf-4795-9270-7ad5e25f67be
⛔ Files ignored due to path filters (4)
apps/mobile/.turbo/turbo-build.logis excluded by!**/*.logapps/web/.turbo/turbo-build.logis excluded by!**/*.loginfra/terraform/.turbo/turbo-build.logis excluded by!**/*.logpackages/shared-types/.turbo/turbo-build.logis excluded by!**/*.log
📒 Files selected for processing (3)
apps/web/src/components/activity/ActivityFeed.tsxpackages/shared-types/tsconfig.jsontsconfig.base.json
✅ Files skipped from review due to trivial changes (1)
- packages/shared-types/tsconfig.json
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/groups/SettlementList.tsx (1)
10-16:⚠️ Potential issue | 🟡 MinorUnsafe cast from
stringtoCurrencyCodeat runtime.The
currencyprop is typed asstring(line 12) but cast toCurrencyCodeat line 62. If a caller passes an unsupported currency code,CURRENCY_SYMBOLS[currency]will returnundefined, producing malformed output likeundefined1,234.56.Consider narrowing the prop type to
CurrencyCode(as done inGroupSummaryPanel.tsxline 38) to catch mismatches at compile time.💡 Suggested fix
type SettlementListProps = { groupId: string; - currency: string; + currency: CurrencyCode; suggestions: SimplifySuggestionDto[]; memberLookup: Record<string, { name: string; email: string }>; initialReminderActivity: ActivityDto[]; };Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/groups/SettlementList.tsx` around lines 10 - 16, The currency prop in SettlementListProps is declared as string but later cast to CurrencyCode when used with CURRENCY_SYMBOLS (in the SettlementList component), which can produce undefined symbols at runtime; change the currency prop type from string to the union type CurrencyCode (importing CurrencyCode where needed) in SettlementListProps and update any callers to pass a valid CurrencyCode so the compiler enforces allowed values, and remove the unsafe cast usage where CURRENCY_SYMBOLS[currency as CurrencyCode] was used.
♻️ Duplicate comments (1)
apps/web/src/components/activity/ActivityFeed.tsx (1)
72-79:⚠️ Potential issue | 🟡 MinorNumeric
amountCentsstill blocks valid fallback tototalAmountCents.The current logic selects
amountCents ?? totalAmountCentsbefore checkingtypeof cents !== 'string'. IfamountCentsis a number (whichformatCurrencyFromCentsaccepts), this returnsnulland also prevents thetotalAmountCentsfallback from being tried.💡 Proposed fix to handle numeric cents
function formatAmount(activity: ActivityDto): string | null { - const cents = activity.metadata?.amountCents ?? activity.metadata?.totalAmountCents; - if (typeof cents !== 'string') return null; + const amountCents = activity.metadata?.amountCents; + const totalAmountCents = activity.metadata?.totalAmountCents; + const cents = + typeof amountCents === 'string' || typeof amountCents === 'number' + ? amountCents + : typeof totalAmountCents === 'string' || typeof totalAmountCents === 'number' + ? totalAmountCents + : null; + if (cents === null) return null; const currency = activity.metadata?.currency; return currency === 'USD' || currency === 'EUR' || currency === 'INR' ? formatCurrencyFromCents(cents, currency) : formatCurrencyFromCents(cents, 'USD'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/activity/ActivityFeed.tsx` around lines 72 - 79, In formatAmount, currently amountCents is chosen first and rejected if it's not a string, which blocks falling back to totalAmountCents when amountCents is numeric; change the selection to try amountCents first but if it's null/undefined then use totalAmountCents, then accept either a number or string for cents (i.e., allow typeof cents === 'number' || typeof cents === 'string'), return null only if neither type matches, and then call formatCurrencyFromCents(cents, currencyOrDefault) with the validated value; use the existing function names formatAmount and formatCurrencyFromCents and the metadata keys amountCents/totalAmountCents and currency to locate the change.
🧹 Nitpick comments (1)
packages/shared-types/src/index.ts (1)
21-26: Consider usingNumber.isSafeIntegerfor safer large-number handling.
Number.isIntegerreturnstruefor integers outside the safe range (>2^53-1), which can silently lose precision when converted to string. UsingNumber.isSafeIntegerwould reject values that cannot be represented exactly.💡 Suggested improvement
if (typeof amountCents === 'number') { - if (!Number.isInteger(amountCents)) { + if (!Number.isSafeInteger(amountCents)) { throw new Error('Currency formatter expects whole cents.'); } return String(amountCents); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-types/src/index.ts` around lines 21 - 26, The current check in the currency formatting branch uses Number.isInteger on the amountCents value which allows integers outside JS safe range; in the function handling amountCents (the branch that returns String(amountCents)), replace Number.isInteger with Number.isSafeInteger to reject unsafe large integers and keep throwing the same error (or update the message to indicate "expects whole safe cents") so callers cannot pass values that would lose precision when converted to string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/components/groups/SettlementList.tsx`:
- Around line 10-16: The currency prop in SettlementListProps is declared as
string but later cast to CurrencyCode when used with CURRENCY_SYMBOLS (in the
SettlementList component), which can produce undefined symbols at runtime;
change the currency prop type from string to the union type CurrencyCode
(importing CurrencyCode where needed) in SettlementListProps and update any
callers to pass a valid CurrencyCode so the compiler enforces allowed values,
and remove the unsafe cast usage where CURRENCY_SYMBOLS[currency as
CurrencyCode] was used.
---
Duplicate comments:
In `@apps/web/src/components/activity/ActivityFeed.tsx`:
- Around line 72-79: In formatAmount, currently amountCents is chosen first and
rejected if it's not a string, which blocks falling back to totalAmountCents
when amountCents is numeric; change the selection to try amountCents first but
if it's null/undefined then use totalAmountCents, then accept either a number or
string for cents (i.e., allow typeof cents === 'number' || typeof cents ===
'string'), return null only if neither type matches, and then call
formatCurrencyFromCents(cents, currencyOrDefault) with the validated value; use
the existing function names formatAmount and formatCurrencyFromCents and the
metadata keys amountCents/totalAmountCents and currency to locate the change.
---
Nitpick comments:
In `@packages/shared-types/src/index.ts`:
- Around line 21-26: The current check in the currency formatting branch uses
Number.isInteger on the amountCents value which allows integers outside JS safe
range; in the function handling amountCents (the branch that returns
String(amountCents)), replace Number.isInteger with Number.isSafeInteger to
reject unsafe large integers and keep throwing the same error (or update the
message to indicate "expects whole safe cents") so callers cannot pass values
that would lose precision when converted to string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0797c1f6-04fd-4d2b-8693-9e5c10c9c519
📒 Files selected for processing (15)
apps/backend/src/activity/activity.service.tsapps/mobile/app/components/ui/MoneyText.tsxapps/mobile/app/screens/ActivityScreen.tsxapps/mobile/app/screens/ExpenseDetailScreen.tsxapps/mobile/app/screens/GuestGroupDetailScreen.tsxapps/mobile/app/screens/HomeScreen.tsxapps/web/app/dashboard/groups/[groupId]/page.tsxapps/web/app/dashboard/page.tsxapps/web/src/components/activity/ActivityFeed.tsxapps/web/src/components/dashboard/ActivityList.tsxapps/web/src/components/groups/GroupSummaryPanel.tsxapps/web/src/components/groups/RecurringExpenseList.tsxapps/web/src/components/groups/SettlementList.tsxpackages/shared-types/src/index.tstsconfig.base.json
✅ Files skipped from review due to trivial changes (3)
- apps/web/app/dashboard/page.tsx
- apps/backend/src/activity/activity.service.ts
- apps/mobile/app/screens/ActivityScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/src/components/dashboard/ActivityList.tsx
- apps/mobile/app/components/ui/MoneyText.tsx
- tsconfig.base.json
- apps/mobile/app/screens/GuestGroupDetailScreen.tsx
- apps/web/app/dashboard/groups/[groupId]/page.tsx
- apps/mobile/app/screens/ExpenseDetailScreen.tsx
- apps/web/src/components/groups/RecurringExpenseList.tsx
- apps/mobile/app/screens/HomeScreen.tsx
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
New Features
Improvements
Tests
Documentation