Implement Splitwise import functionality #264
Merged
Conversation
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
… and settlements.
…ehensive validation with supporting scripts and UI.
…rvice, and introduce various backend scripts for data validation and debugging.
…g, extensive validation scripts, backend expense routes, and related frontend UI updates.
✅ Deploy Preview for split-but-wiser ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughA comprehensive update adding Splitwise data import functionality with OAuth integration, balance caching improvements, currency support across expenses and groups, enhanced analytics and settlement calculation, and corresponding UI/UX updates across mobile and web platforms. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
… checks more effectively
![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: 64
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/app/expenses/routes.py (1)
65-88: Escape search input before using in MongoDB regex queries.The
searchparameter passed toexpense_service.list_group_expensesis used directly in MongoDB$regexoperations (lines 231-235 in service.py) without escaping or normalization. This allows regex injection attacks and ReDoS vulnerabilities. Escape user input withre.escape()before using it in regex queries, or use MongoDB's string matching operators instead of$regex.backend/app/expenses/schemas.py (1)
20-38: Ensure mobile UI renders the new currency, not a hard‑coded symbol.With the new
Currencyenum and currency fields, the UI must render the correct symbol per group/expense. The snippet inmobile/screens/GroupDetailsScreen.js(Line 26:const currency = "₹";) will mislabel USD/EUR groups. Please switch to a currency symbol formatter sourced from the group/expense currency.backend/app/groups/schemas.py (1)
33-36: Consider aligningGroupResponse.currencytype with the enum.
GroupCreateRequestandGroupUpdateRequestuseCurrencyenum, butGroupResponse.currencyremains typed asstr. For consistency and better type safety across the API contract, consider updatingGroupResponseto useCurrencyas well.Pydantic will handle serialization of the enum to its string value automatically.
♻️ Suggested alignment
class GroupResponse(BaseModel): id: str = Field(alias="_id") name: str - currency: str + currency: Currency joinCode: strweb/pages/Groups.tsx (1)
53-55: Avoid usinganytype in getGroupBalance.The callback uses
(g: any)which bypasses type checking. Consider using the proper type.♻️ Suggested fix
const getGroupBalance = (groupId: string): GroupBalanceSummary | undefined => { - return balanceSummary?.groupsSummary?.find((g: any) => g.groupId === groupId); + return balanceSummary?.groupsSummary?.find((g) => g.groupId === groupId); };TypeScript should infer the type from
groupsSummaryif it's properly typed asGroupBalanceSummary[].web/pages/GroupDetails.tsx (1)
723-734: Use a semantic button for the expense card.Replace
motion.divwithmotion.buttonto follow native HTML semantics and W3C best practices. This removes the need for manual keyboard handling (onKeyDown,tabIndex,role="button"), asmotion.buttonprovides built-in keyboard support (Enter and Space activation) automatically.🛠️ Suggested fix
- <motion.div + <motion.button layout key={expense._id} initial={{ opacity: 0, scale: 0.95 }} animate={{ opacity: 1, scale: 1 }} whileHover={{ scale: 1.02 }} onClick={() => openEditExpense(expense)} - onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); openEditExpense(expense); } }} - tabIndex={0} - role="button" + type="button" aria-label={`Expense: ${expense.description}, ${formatCurrency(expense.amount, group?.currency)}`} className={`p-5 flex items-center gap-5 cursor-pointer group relative overflow-hidden ${style === THEMES.NEOBRUTALISM ? 'bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] rounded-none' : 'bg-white/5 border border-white/10 rounded-2xl backdrop-blur-sm hover:bg-white/10 transition-all' }`} > ... - </motion.div> + </motion.button>backend/app/expenses/service.py (2)
57-83: Normalize payer membership checks for mixed ID types.You now support both string and
ObjectIdmembership, butpayer_is_memberstill compares raw values. This can reject valid payers when IDs are stored asObjectIdwhilepaidByis a string. Normalize both sides before comparison.🐛 Proposed fix
- payer_is_member = any( - member.get("userId") == expense_data.paidBy - for member in group.get("members", []) - ) + paid_by_id = str(expense_data.paidBy) + payer_is_member = any( + str(member.get("userId")) == paid_by_id + for member in group.get("members", []) + )
143-168: Guard ObjectId conversion for user IDs in settlement queries.The code passes
user_idsdirectly toObjectId()without validation. If any ID is not a valid ObjectId hex string, this raises an exception and crashes expense creation. The fix filters IDs withObjectId.is_valid()first, which is already used elsewhere in this file.🐛 Proposed fix
- users = await self.users_collection.find( - {"_id": {"$in": [ObjectId(uid) for uid in user_ids]}} - ).to_list(None) + valid_ids = [uid for uid in user_ids if ObjectId.is_valid(uid)] + users = await self.users_collection.find( + {"_id": {"$in": [ObjectId(uid) for uid in valid_ids]}} + ).to_list(None)
🤖 Fix all issues with AI agents
In @.agent/artifacts/balance_caching_plan.md:
- Around line 12-15: The markdown has lint errors (MD022/MD031/MD040) due to
missing blank lines around headings and fenced code blocks and missing fence
language; update the document (e.g., the "### Data Flow" heading and all fenced
blocks) to ensure there is a blank line before and after each heading and fenced
block and add an appropriate language identifier to each code fence (for example
use ```text for plain text blocks) so all occurrences follow the same pattern.
In `@backend/app/auth/service.py`:
- Around line 288-297: The except block catching PyMongoError (the one using
logger.error(...) with user["_id"]) should preserve the original exception chain
when re-raising the HTTPException; change the raise in that block to use "from
e" so the original PyMongoError is chained to the HTTPException (i.e., raise
HTTPException(... ) from e) while keeping the existing logger.error call and
status/details intact.
- Around line 114-140: Wrap the db.users.update_one call in a try/except (or
try/except Exception) so failures are caught and handled similarly to the Google
flow; on exception log or raise an appropriate error and do not proceed to
create a refresh token via _create_refresh_token_record for an un-updated
account. Also compute a single activated_at timestamp once (e.g. activated_at =
datetime.now(timezone.utc)) and reuse that variable in the update_one payload
and when mutating existing_user to avoid mismatched timestamps; ensure
existing_user is only mutated after a successful update.
In `@backend/app/expenses/service.py`:
- Around line 1500-1509: The current loop in the expense prefetch (uses
expense_dates, group_members, member_names and self.users_collection.find_one
with ObjectId) causes N+1 DB queries; replace it with a single query that
fetches all users whose _id is in the group_members set (use {"_id": {"$in":
[...]}} with ObjectId-wrapped ids), then build member_names by iterating the
returned cursor/documents mapping each member_id to user.get("name","Unknown")
and defaulting to "Unknown" for any missing ids; ensure you handle member_id
types (convert strings to ObjectId when building the $in list) so subsequent
code using member_names works unchanged.
- Around line 587-591: The current query fetching settlements (settlements =
await self.settlements_collection.find({"groupId": group_id}).to_list(None))
pulls cancelled entries into balance calculations; update the query used in the
settlement fetches (the find call on self.settlements_collection used around the
balance calculation and similarly at the other occurrence) to exclude cancelled
settlements—e.g., add a filter for status != "cancelled" or explicitly include
only pending/completed statuses—so net balances are computed only from active
settlements.
- Around line 1471-1475: total_paid computation can raise KeyError for missing
"paidBy" and misattribute when IDs have mixed types; update the sum to use
expense.get("paidBy") and expense.get("amount", 0) and compare normalized IDs
(e.g., str(expense.get("paidBy")) == str(member_id)) so missing keys are safe
and ID types are consistent; modify the expression that sets total_paid
(referencing total_paid, expenses, member_id, and the "paidBy"/"amount" keys)
accordingly.
- Around line 1192-1194: Remove the unused local variable groups_map (the dict
comprehension building groups_map = {str(g["_id"]): g.get("name", "Unknown
Group") for g in groups}) from the function—delete that line and any surrounding
comment that solely documents it so there are no unused variables left; ensure
no other code (e.g., later references to groups_map) depends on it and adjust
those references if present.
- Around line 1342-1347: Replace the manual cache-check in the block that reads
group.get("cachedBalances") with the existing helper by calling await
self._get_cached_balances(group_id) to obtain cached balances; remove the
duplicated cache-miss branch that calls _recalculate_group_balances directly so
the single helper (_get_cached_balances) centralizes the cache
retrieval-and-recalc logic and keeps behavior consistent.
- Around line 58-61: Replace the bare except used when converting user_id to an
ObjectId with a narrow exception handler: catch bson.errors.InvalidId (and
TypeError where appropriate) instead of a naked except; specifically update the
try/except around ObjectId(user_id) that assigns user_obj_id so it becomes
except (bson.errors.InvalidId, TypeError): user_obj_id = user_id. Apply the same
change consistently for all occurrences that convert user_id (the six spots
using ObjectId(user_id) which currently use a bare except), and import
bson.errors.InvalidId where needed.
- Around line 230-235: The code in the expenses search branch uses the raw
search string directly in query["$or"] -> {"$regex": search} which risks
catastrophic backtracking and expensive DB scans; update the logic that builds
the query (the block that checks if search and sets query["$or"]) to escape
user-controlled input before using it in $regex (e.g., call a regex-escape
helper or Python's re.escape on the search value), and also enforce a sane
length/character limit on search to prevent overly long patterns; ensure the
updated value is used for both description and tags regex entries.
In `@backend/app/groups/service.py`:
- Around line 312-336: The transaction currently commits even when
db.groups.delete_one({_id: obj_id}) fails; to prevent partial deletions, after
calling db.groups.delete_one (check result.deleted_count) call await
session.abort_transaction() (or raise an exception) when deleted_count != 1 so
the started transaction opened via mongodb.client.start_session() and
session.start_transaction() is explicitly aborted and all prior deletes
(db.expenses.delete_many, db.settlements.delete_many) are rolled back; update
the control flow around result.deleted_count in service.py to abort the session
and return/raise appropriately.
In `@backend/app/integrations/router.py`:
- Around line 31-62: The get_splitwise_oauth_url handler returns the OAuth
secret to the client and never validates state on callback, enabling
CSRF/mix‑up; change get_splitwise_oauth_url to persist the returned secret
server‑side (e.g., Redis/cache with an expiry) keyed to the current_user id or a
short random token and return only a reference token to the client, and then in
the Splitwise callback handler (the code that calls
Splitwise.getOAuth2AuthorizeURL exchange/gets the code) fetch and validate that
persisted state before exchanging the code for tokens; use functions like
set_redis_state/get_redis_state (or a similar store) with TTL and reject the
callback if state missing/mismatched.
- Around line 130-145: Remove the non-existent confirmWarnings field from the
ImportOptions construction in router.py (the ImportOptions(...) passed to
service.start_import) so it only contains valid fields (importReceipts,
importComments, importArchivedExpenses, etc.), and update any call sites that
set status="in_progress" (e.g., where ImportStatus is passed to
service.start_import or similar) to use the enum value ImportStatus.IN_PROGRESS
instead of the raw string; ensure you import ImportStatus from its module if not
already imported.
In `@backend/app/integrations/schemas.py`:
- Around line 55-62: Replace mutable default list/dict fields in the Pydantic
models with Field(default_factory=...) to avoid shared mutable state: for
example in class ImportOptions change selectedGroupIds: list[str] = [] to
selectedGroupIds: list[str] = Field(default_factory=list) (and import Field from
pydantic if not present). Do the same for any other list/dict fields in this
file (the other model attributes noted in the review), using
Field(default_factory=list) or Field(default_factory=dict) as appropriate.
In `@backend/app/integrations/service.py`:
- Around line 233-241: The OAuth secret fields (apiKey and consumerSecret) are
being stored in plaintext in the document inserted by
self.oauth_tokens.insert_one; change this to store either an encrypted value or
a secret reference from your KMS/secrets manager instead of raw
api_key/consumer_secret. Implement or call a helper (e.g., encrypt_secret or
secrets_manager.store_secret) before insertion and save the returned ciphertext
or secret_id in place of apiKey/consumerKey/consumerSecret, and update any
readers (decrypt_secret or secrets_manager.get_secret) that later retrieve these
values so code that uses api_key/consumer_secret reads the decrypted secret at
use-time rather than from the DB.
- Around line 982-1004: The rollback currently unconditionally deletes entities
from mappings; change logic in the loop that iterates mappings to check a flag
(e.g., mapping.get("createdByImport") or mapping["createdByImport"]) and only
call delete_one on self.expenses/self.groups/self.users when that flag is true;
for mappings where createdByImport is false, perform an update_one to unset or
revert fields you set (e.g., remove splitwiseId, importedFrom, importedFields)
instead of deleting; additionally, ensure you also handle related records
created by the job by removing settlements and OAuth tokens tied to the import
(use self.settlements and self.oauth_tokens filtered by importJobId or mapping
identifiers), then continue to delete id_mappings and update import job status
via self.id_mappings.delete_many and self.import_jobs.update_one as before.
- Around line 600-632: The code currently calls
self._increment_summary(import_job_id, "groupsCreated") when an existing_group
is reused, which misreports metrics; modify the reuse branch (the block handling
existing_group and the subsequent id_mappings.insert_one / groups.update_one
calls) to NOT increment "groupsCreated" and instead either increment a new
counter like "groupsReused" via self._increment_summary(import_job_id,
"groupsReused") or omit incrementing entirely, and adjust the checkpoint update
key if you add "groupsReused" (e.g., use the same pattern as the current
_update_checkpoint call but for a "groupsReused.completed" or leave the existing
"groupsImported.completed" increment intact).
- Around line 246-251: The background import is started with
asyncio.create_task(...) but lacks a task-level done callback to handle
cancellations during shutdown; change the call so you capture the Task (task =
asyncio.create_task(...)) and attach a done callback (task.add_done_callback)
that checks task.cancelled() and task.exception(); if cancelled, record and
update the import job status to FAILED using the same helpers (call
self._record_error(...) and the same MongoDB update used in _perform_import) and
if exception is present, log it and persist via self._record_error(...) as well;
ensure the callback swallows/handles exceptions so they don't propagate.
- Around line 682-686: The current call to client.get_expenses(limit=1000) only
retrieves up to 1000 items and will drop remaining expenses; replace it with a
paginated loop using the SDK's offset parameter (calling
client.get_expenses(limit=1000, offset=offset) repeatedly and appending results
until an empty batch) to build all_expenses, then call
self._update_checkpoint(import_job_id, "expensesImported.total",
len(all_expenses)); apply the same offset-based loop when fetching group
expenses so both places fully paginate rather than relying on a single 1000-item
request.
In `@backend/app/integrations/splitwise/client.py`:
- Around line 274-278: The code currently assigns the sentinel string "0" to
group_id when expense.getGroupId() is missing; change this to use None (i.e.,
set group_id = None when hasattr(expense, "getGroupId") is false or
expense.getGroupId() is falsy) and update any downstream code in the import
pipeline that expects a string (e.g., functions that consume group_id or the
import/mapping routine) to treat None as a personal/non‑group expense or map it
to a dedicated "personal" group id; ensure any serialization or DB insert logic
handling group_id accepts nulls or performs the mapping so no invalid "0"
references are created.
In `@backend/migrations/003_add_currency_to_groups.py`:
- Around line 31-55: The code logs the full MongoDB URI (settings.mongodb_url)
which may include credentials; change the logger.info call to log a redacted URI
instead by stripping userinfo (username:password@) before logging: parse
settings.mongodb_url (e.g., with urllib.parse.urlparse or similar) and
reconstruct a safe string without netloc userinfo or replace it with
"<redacted>@host", then call logger.info with that redacted value (keep
AsyncIOMotorClient(settings.mongodb_url) unchanged so the real connection still
uses the full URI) and update the log message near the AsyncIOMotorClient
instantiation.
In `@backend/migrations/004_fix_member_userid_format.py`:
- Around line 119-130: The verification loop currently only checks member userId
types (using groups_with_objectid, cursor, group.get("members", []),
member.get("userId")) and can miss groups where the top-level createdBy wasn't
normalized; update the verification to also inspect group.get("createdBy") for
ObjectId instances, increment an appropriate counter (or reuse
groups_with_objectid) and emit a logger.warning including group.get("name") and
group["_id"] when createdBy is still an ObjectId so the migration verification
fails correctly if createdBy wasn't normalized.
- Around line 35-36: The logger currently prints the raw MongoDB connection
string via logger.info(f"Connecting to MongoDB at {settings.mongodb_url}") which
can leak credentials; update the code that constructs the connection log (before
calling AsyncIOMotorClient and creating client) to redact credentials by parsing
settings.mongodb_url (e.g., use urlparse to remove username/password or strip
the user:pass@ segment) and log a safe host/db-only form instead (reference
logger.info, settings.mongodb_url, AsyncIOMotorClient, client to locate the
code). Ensure the redacted value retains host and database for debugging but
never includes username or password.
In `@backend/migrations/005_init_cached_balances.py`:
- Around line 119-122: The log line currently prints settings.mongodb_url which
may contain credentials; remove or redact credentials before logging. Replace
the logger.info call that references settings.mongodb_url with a safer message
(e.g., log only that a connection is being attempted or log parsed host/port
without user/password) in the block where AsyncIOMotorClient is created and db =
client[settings.database_name] is set; if you need host info, parse
settings.mongodb_url (e.g., via urllib.parse) and exclude username/password
before logging.
In `@backend/migrations/006_fix_split_types.py`:
- Line 24: Remove the unnecessary noqa directive on the import of logger and
settings; update the import statement "from app.config import logger, settings
# noqa: E402" to simply "from app.config import logger, settings" so the unused
E402 suppression is eliminated.
- Line 112: Replace the f-string that explicitly calls str(e) in the migration
error log with the f-string conversion flag `{e!s}` to satisfy RUF010; locate
the logger.error call that currently reads logger.error(f"❌ Migration failed:
{str(e)}") (in the migration error handling block where exception variable `e`
is caught) and update the message to use `{e!s}` instead of `str(e)` so the
exception is converted via the f-string conversion flag.
- Line 150: Replace the explicit str() conversion inside the f-string at the
logger.error call that logs "❌ Verification failed" so it uses the f-string
conversion flag; change the message from f"❌ Verification failed: {str(e)}" to
f"❌ Verification failed: {e!s}" to satisfy the RUF010 rule (locate the
logger.error call with the "❌ Verification failed" text in the migration).
- Around line 134-136: The three logger.info calls using f-strings contain no
placeholders; remove the unnecessary f prefix on the string literals used in
logger.info (the calls invoking logger.info with "\n📊 Verification Results:", "
Total expenses: {total_expenses}" and "\n Sample verification:" — note keep
the f only for the middle line if you intend to interpolate total_expenses,
otherwise remove f on that one too) so that logger.info is passed plain strings
or a proper formatted string when interpolation is needed; update the
logger.info invocations accordingly where the function name logger.info appears
in the diff.
- Around line 98-101: The except block that currently catches a broad Exception
in the migration (the except Exception as e handling an expense with variables
logger, description, and error_count) should include the exception type or full
traceback to aid diagnostics; update the handler to log e.__class__.__name__ or
use logger.exception/traceback to include stacktrace and exception type in the
message (e.g., "Failed to process expense {description}: {e} ({type})"), leaving
the error_count increment and continue behavior unchanged.
- Line 58: The current logger.info call logs settings.mongodb_url which may
include credentials; update the migration to avoid exposing secrets by masking
or omitting sensitive parts before logging: create a helper (e.g., mask_url)
that uses urllib.parse.urlparse to extract scheme, hostname, port and path and
replace userinfo with asterisks (or simply log only hostname and port), then
call logger.info with the masked URL instead of settings.mongodb_url; locate the
logger.info(...) line in 006_fix_split_types.py and update it to use the new
mask_url helper.
- Around line 145-147: The log construction slices expense.get('description')
which can be None and causes a TypeError; update the code in the same area where
the message is built (used by fix_split_types) to coalesce a default string
before slicing — e.g., use (expense.get('description') or "")[:30] or similar —
so the expression f"{expense.get('description')[:30]:30}" never operates on None
and matches the defaulting behavior used elsewhere in fix_split_types.
In `@backend/requirements.txt`:
- Line 14: The splitwise dependency is using a minimum-version specifier; update
the requirements entry for the package named "splitwise" to use an exact pinned
version by replacing the ">=" specifier with "==" and set the version to 3.0.0
(i.e., change splitwise>=3.0.0 to splitwise==3.0.0) so it matches the project's
exact-version pinning convention in requirements.txt.
In `@backend/tests/test_settlement_calculation.py`:
- Around line 82-103: The code currently computes settlement_amount then only
rounds it when appending to settlements, causing drift because remaining_debt
and remaining_credit are decremented by the unrounded value; change the logic in
the loop (where settlement_amount, remaining_debt, remaining_credit, and
settlements are used) to compute a single rounded value (e.g., rounded_amount =
round(settlement_amount, 2)), use rounded_amount for the >0.01 check and for the
"amount" sent to settlements, and subtract rounded_amount from remaining_debt
and from the creditor's credit in remaining_credit so all arithmetic uses the
same rounded value and avoids cumulative float drift.
In `@backend/tests/test_splitwise_import.py`:
- Around line 118-120: Several tests repeatedly import SplitwiseClient inside
test methods; move the repeated import(s) to the module level to avoid
duplication and improve clarity by adding a single module-level binding like
SplitwiseClient = ... at top of the test file, or if import availability is a
concern use
pytest.importorskip("app.integrations.splitwise.client").SplitwiseClient; update
test_simple_expense_transform and the other tests that reference SplitwiseClient
(lines noted around 160, 191, 227, 239, 257, 264, 270) to use the module-level
symbol instead of importing inside each test.
- Around line 440-622: Add an integration test in
backend/tests/test_splitwise_import.py that calls the actual service method
_import_expenses() (from backend/app/integrations/service.py) instead of
re-implementing the paidShare logic inline; create mock Splitwise expense
objects that exercise the scenarios in the unit tests (normal expense,
self-paid, solo, no payers, empty) and assert the persisted/imported expense
payer equals the expected userId (verifying the service uses max(mapped_shares,
key=lambda s: s["paidShare"]) as implemented in _import_expenses); ensure you
mock external calls (API/network/db) the service depends on and reuse existing
test fixtures/helpers for invoking _import_expenses and inspecting the resulting
imported expense records.
In `@backend/verify_settlements.py`:
- Around line 119-123: The create_group function currently returns None on HTTP
failure despite being annotated to return str; change it to raise an exception
on non-201 responses so callers don't receive a None and build invalid URLs.
Inside create_group, after detecting response.status_code != 201 (the block that
currently prints and returns None), raise a descriptive exception (e.g.,
RuntimeError or a custom exception) including response.status_code and
response.text; keep the success path returning the group id string so the
function signature remains correct and callers of create_group will fail fast
with a clear error instead of continuing with None.
- Around line 32-33: The cleanup function is too dangerous when API_URL is
misconfigured; update the module so cleanup_test_data first validates API_URL
and requires an explicit override before performing destructive calls: add a
check that API_URL points to a safe host (e.g., localhost/127.0.0.1) and/or
require an environment variable toggle (e.g., ALLOW_DESTRUCTIVE_CLEANUP=true) to
be set, and make cleanup_test_data abort (raise/return) unless the guard passes;
reference API_URL and cleanup_test_data when implementing the validation and the
early-return/exception behavior.
In `@docs/splitwise-import-integration.md`:
- Around line 24-36: The markdown in docs/splitwise-import-integration.md
contains fenced code blocks (the ASCII diagram block shown in the diff) without
language identifiers; update each fenced block (e.g., the diagram block) to
include an appropriate language tag such as `text` (for ASCII diagrams), or
`http`/`javascript`/`python` as relevant, so markdownlint MD040 passes, then
re-run the linter to verify; target the fenced block around the Splitwise API
diagram in that file.
- Around line 624-633: The schema for the import_jobs collection currently
includes splitwiseAccessToken which duplicates sensitive OAuth data; instead
update the docs and schema to remove splitwiseAccessToken and replace it with a
reference (e.g., splitwiseTokenId or oauthTokenRef) that points to the
oauth_tokens collection where the encrypted token is stored, ensure the new
field name is documented and described, and note that import_jobs must only hold
the token ID/reference (not raw token) and that oauth_tokens remains the single
source of truth for encryption and rotation.
- Around line 264-288: Extract the hard-coded CALLS_PER_HOUR constant into a
configurable default (e.g., load from environment variable or config with a
fallback) and update the rate-limited wrapper (call_splitwise_api and ONE_HOUR
usage) to read that configured value at runtime; ensure the code
validates/normalizes the configured value (integer > 0) and documents the config
key so callers can override the default without changing the function.
In `@mobile/api/client.js`:
- Around line 124-126: The handleSplitwiseCallback function only sends { code,
state } but must match the web API signature by accepting optional
selectedGroupIds and accessToken and including them in the POST payload; update
the handleSplitwiseCallback(code, state, selectedGroupIds, accessToken)
signature, ensure the apiClient.post("/import/splitwise/callback", { code,
state, selectedGroupIds, accessToken }) payload is sent (omitting undefined
values if desired), and update any callers to pass selectedGroupIds/accessToken
as needed so mobile and web behaviors are consistent.
- Around line 128-138: The POST retry behavior in apiClient causes
startSplitwiseImport to potentially create duplicate jobs and rollbackImport to
act on already-rolled-back jobs; update startSplitwiseImport and rollbackImport
to include an idempotency key (e.g., generate/store a client-provided
idempotency key per user/provider) sent in headers or body, or change
startSplitwiseImport to first call a new GET/check endpoint to detect an
existing pending/in-progress import before POSTing, and update rollbackImport to
validate job state before issuing delete/rollback; modify the functions
startSplitwiseImport and rollbackImport to attach the idempotency header (or
perform the pre-check via getImportStatus) so automatic retries from apiClient
do not create duplicate imports or re-rollback completed jobs.
In `@mobile/screens/SplitwiseImportScreen.js`:
- Around line 143-167: Remove the dead style definitions from the component's
StyleSheet: delete the unused style keys input, link, progressContainer,
progressHeader, progressText, and progressBar from the StyleSheet.create block
(and remove any related unused imports if they become unused afterward); before
removing, search the component for any references to these style names to ensure
nothing else uses them and update or consolidate remaining styles as needed.
In `@web/components/AnalyticsContent.tsx`:
- Around line 220-222: Duplicate inline color arrays are used for the top
categories cells (seen in the analytics.topCategories.map block and again around
the other map at lines ~350-352); extract a single exported constant such as
TOP_CATEGORY_COLORS (or COLORS_TOP_CATEGORIES) and replace both inline arrays
with that constant referenced in the Cell fill property, and resolve the palette
discrepancy by choosing the final color (decide between `#f97316` vs `#8b5cf6`) so
both usages are identical.
- Around line 322-327: The YAxis tickFormatter is using a plain template string
with groupCurrency causing inconsistent currency display; update the YAxis
tickFormatter to call the existing formatCurrency helper (use
formatCurrency(value, groupCurrency) or the project's signature) instead of
`${groupCurrency} ${value.toLocaleString()}` so ticks match other components;
ensure formatCurrency is imported/available in AnalyticsContent.tsx and that
value is passed as a number to the formatter (reference YAxis, tickFormatter,
formatCurrency, groupCurrency).
In `@web/pages/Dashboard.tsx`:
- Around line 107-121: CustomPieTooltip is recreated on every render inside
Dashboard; extract it outside the Dashboard component (or wrap it with
React.memo) and accept style as a prop so it no longer closes over Dashboard
scope. Update the component signature (e.g., CustomPieTooltip({ active, payload,
style })) and adjust where you render the tooltip to pass the current style prop
from Dashboard; this ensures the tooltip is stable across renders and improves
performance.
- Around line 350-355: The progress bar width can become NaN/Infinity when
summary.totalOwedToYou + summary.totalYouOwe is zero; in the component that
renders the bar (references: isPositive, yourBalance, summary.totalOwedToYou,
summary.totalYouOwe) compute a safe percentage first (e.g. const denom =
summary.totalOwedToYou + summary.totalYouOwe; const pct = denom > 0 ?
(yourBalance / denom) * 100 : 0), clamp it to a 0–100 range (Math.max(0,
Math.min(100, pct))) and use that clamped value for the style width instead of
performing the division inline.
- Around line 59-67: Replace the untyped lookup parameter in the
groupsWithBalance mapping by using the proper BalanceSummary groups type: import
the GroupBalanceSummary (or equivalent) from ../types, type
summaryRes.data.groupsSummary as GroupBalanceSummary[] and change the callback
typing from (g: any) to (g: GroupBalanceSummary) so the find call and the
resulting groupBalance variable are properly typed; update any refs to
g.group_id and yourBalanceInGroup accordingly to satisfy the compiler and remove
the use of any in the groupsWithBalance computation.
In `@web/pages/Friends.tsx`:
- Around line 95-97: The backend must include a currency field on each breakup
item returned by get_friends_balance_summary; once that change is deployed,
update the frontend GroupBreakdown interface to add currency and then change the
formatPrice function (used by GroupBreakdown rendering) to accept/consume that
currency and call formatCurrency(amount, currency) (or pass currency as the
second arg) instead of hardcoding USD; also update places that construct/consume
GroupBreakdown so they pass through the new currency value from the API (e.g.,
where GroupBreakdown objects are created or mapped).
In `@web/pages/GroupDetails.tsx`:
- Around line 136-138: The analytics fetch useEffect(s) (the one that currently
depends on activeTab/timeframe) can show stale data when navigating groups;
update the useEffect dependencies to include id so fetchData is re-run on group
change. Locate the useEffect that calls fetchData (and the similar effect around
the analytics logic referenced later) and add id to the dependency array
alongside activeTab and timeframe, ensuring fetchData uses the current id to
refetch analytics when group changes.
- Around line 205-214: The loadMoreExpenses function ignores the active search
by calling getExpenses(id, nextPage) without the current debouncedSearch; update
loadMoreExpenses to pass the search term (debouncedSearch) into getExpenses
(e.g., getExpenses(id, nextPage, debouncedSearch)) so additional pages honor the
active filter, and ensure state updates (setExpenses, setPage, setHasMore)
continue to operate on the filtered result set returned by getExpenses.
- Around line 103-134: The groupTotals useMemo zeroes per-user metrics whenever
totalSummary exists; update the logic inside useMemo (referencing groupTotals,
totalSummary, myContribution, myShare, netBalance) to: if totalSummary contains
per-user fields (e.g., totalSummary.myContribution, totalSummary.myShare,
totalSummary.netBalance) use those values, otherwise compute
myContribution/myShare/netBalance from the full expenses array as the current
fallback (same computation used when totalSummary is falsy); ensure the return
merges totalSummary.totalAmount and expenseCount with either backend-provided
per-user values or the computed fallbacks and keep the same dependency list for
useMemo (including user?._id).
In `@web/pages/SplitwiseCallback.tsx`:
- Around line 120-122: The empty catch in the polling logic of the
SplitwiseCallback component is swallowing errors; update the catch to at minimum
log the error in non-production (e.g., when process.env.NODE_ENV !==
'production' or using the app logger) and implement retry controls: add a
maxAttempts counter and increment it on each failure, stop retrying and
surface/log a final error once maxAttempts is reached, and apply a short
delay/backoff between retries; locate the polling function or useEffect that
contains the try/catch in SplitwiseCallback.tsx and modify that catch block and
surrounding logic accordingly.
- Around line 88-124: The startProgressPolling function creates a setInterval
that isn't cleared when the component unmounts, causing potential memory leaks
and setState on unmounted components; fix by storing the interval ID in a ref or
outer variable and ensuring you clearInterval(intervalId) in a useEffect cleanup
(and also on any early exits: when status becomes 'completed'/'failed' or in the
catch block) so that setProgress, setStatus and setImporting are never called
after unmount. Ensure the same intervalId is referenced in clearInterval calls
inside startProgressPolling and in the useEffect return cleanup.
- Around line 35-45: The inner variable named "state" inside the handleCallback
function shadows the outer "state" variable declared earlier; rename the inner
variable (e.g., callbackState or urlState) used for URLSearchParams.get('state')
in handleCallback to avoid shadowing and update all references in that function
(handleCallback, and any subsequent uses within that scope) so the outer state
remains unaffected and the intent is clearer.
In `@web/pages/SplitwiseGroupSelection.tsx`:
- Around line 178-195: The group card motion.div is not keyboard-accessible;
update the clickable element (the motion.div rendered for each group using
key={group.splitwiseId}) to be focusable and operable via keyboard: either
replace it with a semantic interactive element (button or input type="checkbox")
or add tabIndex={0}, onKeyDown handler that calls toggleGroup(group.splitwiseId)
for Enter/Space, and role="button" or role="checkbox" with
aria-checked={isSelected} to expose selection state; ensure visual focus styles
remain and keep the existing onClick behavior and Check rendering tied to
isSelected.
In `@web/pages/SplitwiseImport.tsx`:
- Around line 56-64: The button currently rendered in SplitwiseImport (the
element invoking handleOAuthImport and using props loading and isNeo) lacks an
explicit type and can accidentally submit a surrounding form; update the main
button element to include type="button" to prevent implicit form submission
while preserving existing onClick, disabled, and className behavior.
- Around line 88-112: The inline SVG icons in SplitwiseImport.tsx (the checkmark
SVG inside the first h3 and the warning triangle SVG in the Important Notes h3)
lack accessibility metadata; update each SVG: if decorative, add
aria-hidden="true" and focusable="false" (and remove role), otherwise include a
<title> element and aria-label/role="img" with descriptive text; ensure both SVG
elements are updated accordingly so screen readers either ignore decorative
icons or announce meaningful labels.
- Around line 15-26: In handleOAuthImport, guard against a missing or invalid
authorization_url from getSplitwiseAuthUrl(): after extracting authorization_url
from response.data, check its presence/truthiness before setting
window.location.href; if it's falsy, call addToast with a clear error message,
console.error the response for debugging, and call setLoading(false) to reset
the UI instead of redirecting to undefined. Ensure this check is applied inside
the try block before the redirect so the user isn't left stuck in loading when
the backend returns a malformed response.
In `@web/services/api.ts`:
- Around line 41-59: The code currently persists access and refresh tokens to
localStorage (lines around localStorage.setItem in the axios refresh flow),
which is XSS‑readable; instead, stop writing tokens to localStorage and move to
a cookie + memory approach: have the server issue a Secure, HttpOnly refresh
cookie and keep the short‑lived access token only in an in‑memory store (e.g.,
an authService.get/setAccessToken backing variable) so that in this file you set
originalRequest.headers.Authorization = `Bearer ${accessTokenFromMemory}` and
update that in-memory token when the refresh response returns; remove
localStorage.setItem/removeItem calls (and the localStorage.removeItem calls on
refresh failure) and instead trigger logout/redirect while clearing in-memory
token, and ensure the refresh POST does not rely on client JS to set the refresh
token (server cookie will be sent automatically).
- Around line 111-113: getDashboardAnalytics currently calls the friends-balance
endpoint; change it to call the backend analytics endpoint instead (or rename if
the function should remain friends balance). Update the exported function
getDashboardAnalytics to accept an optional groupId (or require groupId if
backend needs it) and call `/groups/${groupId}/analytics` (or
`/groups/analytics` if your backend exposes an all-groups analytics route),
referencing the backend route group_expense_analytics; if the original function
was meant to return friends balance, rename it to getFriendsBalance to avoid
duplication.
- Around line 115-135: Remove the unused getSplitwisePreview function and any
callers; for preview flows call handleSplitwiseCallback(code?, state?) without
passing selectedGroupIds so the backend returns preview data (i.e., rely on
handleSplitwiseCallback to POST to /import/splitwise/callback with no
payload.options to get preview). Also change startSplitwiseImport to send a
StartImportRequest-shaped payload (e.g., { provider: 'splitwise', options: {/*
if needed */} }) rather than { api_key }, since the backend expects
provider/options and reads credentials from env variables; update any callers of
startSplitwiseImport accordingly.
In `@web/types.ts`:
- Around line 84-90: The FriendBalance TypeScript interface currently declared
as FriendBalance (fields friendId, userName, netBalance, owedToYou, youOwe) does
not match the backend model (userId, owesYou, breakdown, lastActivity); update
the interface to match the backend shape or add a mapping layer in the API
client that adapts the backend response to the existing frontend shape.
Specifically either (A) change the interface fields to userId, owesYou,
breakdown, lastActivity with appropriate types to mirror
backend/app/expenses/schemas.py, or (B) implement a transformer in the API call
that converts backend userId→friendId, owesYou→owedToYou/youOwe (and include
breakdown and lastActivity as needed) and return the current FriendBalance
shape; locate the FriendBalance interface in web/types.ts and the API response
handling code to perform the mapping.
In `@web/utils/formatters.ts`:
- Around line 9-11: The current isValidCurrencyCode function uses the "in"
operator which returns true for inherited prototype keys (e.g., "toString");
update the check to ensure only own properties of CURRENCIES are accepted by
replacing `code in CURRENCIES` with
`Object.prototype.hasOwnProperty.call(CURRENCIES, code)` (keep the function
signature `isValidCurrencyCode(code: string): code is CurrencyCode` so the type
guard remains).
Comment on lines
+12
to
+15
| ### Data Flow | ||
| ``` | ||
| Expense Created → Settlements Created → Recalculate balances on every request | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdownlint errors (headings/fences need spacing + language).
MD022/MD031/MD040 are triggered throughout the doc. Add blank lines around headings and fenced blocks, and specify a language on fences. Example fix (apply consistently):
🧹 Example fix
-### Data Flow
-```
-Expense Created → Settlements Created → Recalculate balances on every request
-```
+### Data Flow
+
+```text
+Expense Created → Settlements Created → Recalculate balances on every request
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Data Flow | |
| ``` | |
| Expense Created → Settlements Created → Recalculate balances on every request | |
| ``` | |
| ### Data Flow | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
13-13: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.agent/artifacts/balance_caching_plan.md around lines 12 - 15, The markdown
has lint errors (MD022/MD031/MD040) due to missing blank lines around headings
and fenced code blocks and missing fence language; update the document (e.g.,
the "### Data Flow" heading and all fenced blocks) to ensure there is a blank
line before and after each heading and fenced block and add an appropriate
language identifier to each code fence (for example use ```text for plain text
blocks) so all occurrences follow the same pattern.
Comment on lines
+114
to
+140
| await db.users.update_one( | ||
| {"_id": existing_user["_id"]}, | ||
| { | ||
| "$set": { | ||
| "hashed_password": hashed_password, | ||
| "name": name, # Update with new name if provided | ||
| "isPlaceholder": False, | ||
| "auth_provider": "email", | ||
| "activated_at": datetime.now(timezone.utc), | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| # Return the linked account | ||
| # Update in-memory user object to match DB state | ||
| existing_user["hashed_password"] = hashed_password | ||
| existing_user["name"] = name | ||
| existing_user["isPlaceholder"] = False | ||
| existing_user["auth_provider"] = "email" | ||
| existing_user["activated_at"] = datetime.now(timezone.utc) | ||
|
|
||
| # Create refresh token | ||
| refresh_token = await self._create_refresh_token_record( | ||
| str(existing_user["_id"]) | ||
| ) | ||
|
|
||
| return {"user": existing_user, "refresh_token": refresh_token} |
Contributor
There was a problem hiding this comment.
Missing error handling for database update and timestamp inconsistency.
Two issues in the placeholder activation flow:
-
No try/except around
update_one: Unlike the Google auth flow (lines 283-297), this database operation lacks error handling. If the update fails, the function continues with stale data and creates a refresh token for a non-activated account. -
activated_atcomputed twice: The timestamp is generated separately for the DB update (line 122) and in-memory object (line 133), causing a potential mismatch.
Proposed fix
- await db.users.update_one(
- {"_id": existing_user["_id"]},
- {
- "$set": {
- "hashed_password": hashed_password,
- "name": name, # Update with new name if provided
- "isPlaceholder": False,
- "auth_provider": "email",
- "activated_at": datetime.now(timezone.utc),
- }
- },
- )
-
- # Return the linked account
- # Update in-memory user object to match DB state
- existing_user["hashed_password"] = hashed_password
- existing_user["name"] = name
- existing_user["isPlaceholder"] = False
- existing_user["auth_provider"] = "email"
- existing_user["activated_at"] = datetime.now(timezone.utc)
+ activated_at = datetime.now(timezone.utc)
+ update_data = {
+ "hashed_password": hashed_password,
+ "name": name,
+ "isPlaceholder": False,
+ "auth_provider": "email",
+ "activated_at": activated_at,
+ }
+ try:
+ await db.users.update_one(
+ {"_id": existing_user["_id"]},
+ {"$set": update_data},
+ )
+ existing_user.update(update_data)
+ except PyMongoError as e:
+ logger.error(
+ "Failed to activate placeholder account for user_id=%s: %s",
+ existing_user["_id"],
+ str(e),
+ )
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Failed to activate account",
+ ) from e🤖 Prompt for AI Agents
In `@backend/app/auth/service.py` around lines 114 - 140, Wrap the
db.users.update_one call in a try/except (or try/except Exception) so failures
are caught and handled similarly to the Google flow; on exception log or raise
an appropriate error and do not proceed to create a refresh token via
_create_refresh_token_record for an un-updated account. Also compute a single
activated_at timestamp once (e.g. activated_at = datetime.now(timezone.utc)) and
reuse that variable in the update_one payload and when mutating existing_user to
avoid mismatched timestamps; ensure existing_user is only mutated after a
successful update.
Comment on lines
288
to
+297
| except PyMongoError as e: | ||
| logger.warning("Failed to update user profile: %s", str(e)) | ||
| logger.error( | ||
| "Failed to activate placeholder account for user_id=%s: %s", | ||
| user["_id"], | ||
| str(e), | ||
| ) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to activate account", | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Chain the original exception for better debugging.
Per Python best practices and static analysis (B904), exceptions raised within an except block should use from e to preserve the exception chain for debugging.
Proposed fix
except PyMongoError as e:
logger.error(
"Failed to activate placeholder account for user_id=%s: %s",
user["_id"],
str(e),
)
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Failed to activate account",
- )
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except PyMongoError as e: | |
| logger.warning("Failed to update user profile: %s", str(e)) | |
| logger.error( | |
| "Failed to activate placeholder account for user_id=%s: %s", | |
| user["_id"], | |
| str(e), | |
| ) | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to activate account", | |
| ) | |
| except PyMongoError as e: | |
| logger.error( | |
| "Failed to activate placeholder account for user_id=%s: %s", | |
| user["_id"], | |
| str(e), | |
| ) | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to activate account", | |
| ) from e |
🧰 Tools
🪛 Ruff (0.14.14)
294-297: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In `@backend/app/auth/service.py` around lines 288 - 297, The except block
catching PyMongoError (the one using logger.error(...) with user["_id"]) should
preserve the original exception chain when re-raising the HTTPException; change
the raise in that block to use "from e" so the original PyMongoError is chained
to the HTTPException (i.e., raise HTTPException(... ) from e) while keeping the
existing logger.error call and status/details intact.
Comment on lines
+312
to
+336
| # Use transaction to ensure atomicity of all deletions | ||
| # If any delete fails, all changes are rolled back | ||
| async with await mongodb.client.start_session() as session: | ||
| async with session.start_transaction(): | ||
| # Delete expenses | ||
| # Note: groupId in expenses is stored as string | ||
| await db.expenses.delete_many({"groupId": group_id}, session=session) | ||
|
|
||
| # Delete settlements | ||
| await db.settlements.delete_many({"groupId": group_id}, session=session) | ||
|
|
||
| # Delete the group itself | ||
| result = await db.groups.delete_one({"_id": obj_id}, session=session) | ||
|
|
||
| if result.deleted_count == 1: | ||
| if is_imported: | ||
| # Remove ID mapping for this group | ||
| # We do NOT remove the user mappings because users might be in other groups | ||
| # We do NOT remove import jobs because history is useful | ||
| await db.splitwise_id_mappings.delete_one( | ||
| {"entityType": "group", "splitwiserId": group_id}, | ||
| session=session, | ||
| ) | ||
|
|
||
| return True |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Motor MongoDB async transaction context manager commit behavior on normal exit
💡 Result:
In Motor (asyncio), session.start_transaction() is an async context manager that auto-finalizes the transaction:
- If the
async with session.start_transaction():block exits normally (no exception), Motor/PyMongo automatically callscommit_transaction(). [1][2] - If the block exits with an exception, it automatically calls
abort_transaction(). [1][2]
So if you enter the block and don’t raise an exception (and don’t explicitly abort), the transaction will be committed on exit, even if you never call commit_transaction() yourself. [1][3]
To avoid committing, you must either:
- call
await session.abort_transaction()before leaving the block, or - raise an exception so the context manager aborts. [1][2]
Related note: end_session() aborts only if a transaction is still active; it won’t undo a transaction that already auto-committed when the start_transaction() block ended. [4]
Sources: PyMongo async session docs on start_transaction() auto-commit/abort [1], Motor docs example stating normal-exit commits and exception aborts [2], MongoDB Community thread confirming this behavior in Motor [3], Motor session docs for end_session() behavior [4].
🌐 Web query:
Motor Python MongoDB session abort_transaction rollback
💡 Result:
In Motor, there’s no separate “rollback” API—rolling back a transaction == aborting it.
await session.abort_transaction()ends the active transaction and discards all writes done in that transaction (nothing becomes visible/committed). This maps to MongoDB’sabortTransactionbehavior. [1]- If you use
async with session.start_transaction():as a context manager, it auto-commits if the block exits normally, and auto-aborts if the block exits with an exception. [2] await session.end_session()will also abort any active transaction. [3]
Typical Motor pattern (abort/rollback on error)
async with await client.start_session() as session:
try:
async with session.start_transaction():
await coll1.insert_one({"_id": 1}, session=session)
await coll2.update_one({"_id": 2}, {"$set": {"x": 1}}, session=session)
# normal exit => commit
except Exception:
# exception exit => abort (rollback) automatically
raiseIf you want to force rollback based on a condition
async with await client.start_session() as session:
async with session.start_transaction():
await coll.insert_one({"_id": 1}, session=session)
if not ok:
await session.abort_transaction() # rollback
raise RuntimeError("forcing rollback") # ensure block doesn't exit “normally”Note: Aborting only affects operations executed inside that transaction/session. Writes done outside a transaction won’t be rolled back automatically. [1]
Sources: MongoDB abortTransaction command semantics [1], Motor transaction context manager behavior [2], Motor end_session() behavior [3].
[1] https://www.mongodb.com/docs/current/reference/command/abortTransaction/
[2] https://motor.readthedocs.io/en/3.3.1/api-asyncio/asyncio_motor_client.html
[3] https://motor.readthedocs.io/en/3.3.1/api-asyncio/asyncio_motor_client_session.html
🏁 Script executed:
# Let's examine the actual file to understand the context better
find . -name "service.py" -path "*/groups/*" | head -5Repository: Devasy/splitwiser
Length of output: 92
🏁 Script executed:
cat -n backend/app/groups/service.py | head -350 | tail -80Repository: Devasy/splitwiser
Length of output: 3845
Abort the transaction when group deletion fails to prevent partial data loss.
If groups.delete_one() fails to delete the group (deleted_count != 1), expenses and settlements are already deleted within the transaction. Without an explicit abort, the async with session.start_transaction(): block exits normally and Motor automatically commits, leaving the database in an inconsistent state where the group remains while its related data is deleted.
🐛 Proposed fix
result = await db.groups.delete_one({"_id": obj_id}, session=session)
-
- if result.deleted_count == 1:
- if is_imported:
- # Remove ID mapping for this group
- # We do NOT remove the user mappings because users might be in other groups
- # We do NOT remove import jobs because history is useful
- await db.splitwise_id_mappings.delete_one(
- {"entityType": "group", "splitwiserId": group_id},
- session=session,
- )
-
- return True
+ if result.deleted_count != 1:
+ await session.abort_transaction()
+ return False
+
+ if is_imported:
+ # Remove ID mapping for this group
+ # We do NOT remove the user mappings because users might be in other groups
+ # We do NOT remove import jobs because history is useful
+ await db.splitwise_id_mappings.delete_one(
+ {"entityType": "group", "splitwiserId": group_id},
+ session=session,
+ )
+
+ return True🤖 Prompt for AI Agents
In `@backend/app/groups/service.py` around lines 312 - 336, The transaction
currently commits even when db.groups.delete_one({_id: obj_id}) fails; to
prevent partial deletions, after calling db.groups.delete_one (check
result.deleted_count) call await session.abort_transaction() (or raise an
exception) when deleted_count != 1 so the started transaction opened via
mongodb.client.start_session() and session.start_transaction() is explicitly
aborted and all prior deletes (db.expenses.delete_many,
db.settlements.delete_many) are rolled back; update the control flow around
result.deleted_count in service.py to abort the session and return/raise
appropriately.
Comment on lines
+31
to
+55
| logger.info(f"Connecting to MongoDB at {settings.mongodb_url}") | ||
| client = AsyncIOMotorClient(settings.mongodb_url) | ||
| db = client[settings.database_name] | ||
| groups_collection = db["groups"] | ||
|
|
||
| # Find groups without currency field or with null currency | ||
| groups_without_currency = await groups_collection.count_documents( | ||
| {"$or": [{"currency": {"$exists": False}}, {"currency": None}]} | ||
| ) | ||
|
|
||
| if groups_without_currency == 0: | ||
| logger.info("✅ All groups already have a currency field") | ||
| return | ||
|
|
||
| logger.info(f"Found {groups_without_currency} groups without currency field") | ||
| logger.info("Adding default currency (USD) to these groups...") | ||
|
|
||
| # Update all groups without currency | ||
| result = await groups_collection.update_many( | ||
| {"$or": [{"currency": {"$exists": False}}, {"currency": None}]}, | ||
| {"$set": {"currency": "USD"}}, | ||
| ) | ||
|
|
||
| logger.info(f"✅ Successfully updated {result.modified_count} groups") | ||
| logger.info("Groups now have default currency set to USD") |
Contributor
There was a problem hiding this comment.
Avoid logging MongoDB credentials.
Line 31 logs the full connection URI; if it contains username/password, this leaks secrets into logs. Please redact credentials before logging.
🔒 Proposed fix (redact credentials in logs)
@@
-import asyncio
-import sys
-from pathlib import Path
+import asyncio
+import sys
+from pathlib import Path
+from urllib.parse import urlparse, urlunparse
@@
from app.config import logger, settings # noqa: E402
+
+def _redact_mongo_url(url: str) -> str:
+ parsed = urlparse(url)
+ if parsed.username or parsed.password:
+ host = parsed.hostname or ""
+ if parsed.port:
+ host = f"{host}:{parsed.port}"
+ return urlunparse(parsed._replace(netloc=host))
+ return url
@@
- logger.info(f"Connecting to MongoDB at {settings.mongodb_url}")
+ logger.info(f"Connecting to MongoDB at {_redact_mongo_url(settings.mongodb_url)}")🤖 Prompt for AI Agents
In `@backend/migrations/003_add_currency_to_groups.py` around lines 31 - 55, The
code logs the full MongoDB URI (settings.mongodb_url) which may include
credentials; change the logger.info call to log a redacted URI instead by
stripping userinfo (username:password@) before logging: parse
settings.mongodb_url (e.g., with urllib.parse.urlparse or similar) and
reconstruct a safe string without netloc userinfo or replace it with
"<redacted>@host", then call logger.info with that redacted value (keep
AsyncIOMotorClient(settings.mongodb_url) unchanged so the real connection still
uses the full URI) and update the log message near the AsyncIOMotorClient
instantiation.
Comment on lines
+178
to
+195
| return ( | ||
| <motion.div | ||
| key={group.splitwiseId} | ||
| initial={{ x: -20, opacity: 0 }} | ||
| animate={{ x: 0, opacity: 1 }} | ||
| onClick={() => toggleGroup(group.splitwiseId)} | ||
| className={`transition-all cursor-pointer p-4 ${isNeo | ||
| ? `bg-white border-2 border-black rounded-none shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] hover:shadow-none hover:translate-x-[2px] hover:translate-y-[2px] ${isSelected ? 'bg-blue-50' : ''}` | ||
| : `bg-white dark:bg-gray-800 rounded-xl shadow hover:shadow-md border-2 ${isSelected ? 'border-blue-500' : 'border-transparent'}` | ||
| }`} | ||
| > | ||
| <div className="flex items-center gap-4"> | ||
| {/* Checkbox */} | ||
| <div className={`flex-shrink-0 w-6 h-6 flex items-center justify-center transition-all ${isSelected | ||
| ? (isNeo ? 'bg-black' : 'bg-blue-500') | ||
| : 'bg-white' | ||
| } ${isNeo ? 'border-2 border-black rounded-none' : 'border-2 border-gray-300 rounded-md'}`}> | ||
| {isSelected && <Check className="w-4 h-4 text-white stroke-[3]" />} |
Contributor
There was a problem hiding this comment.
Make selectable group cards keyboard-accessible.
The clickable motion.div blocks keyboard-only users from selecting groups. Add focusability and key handlers (or use a button/checkbox element) and expose selection state via ARIA.
♿ Suggested fix
<motion.div
key={group.splitwiseId}
initial={{ x: -20, opacity: 0 }}
animate={{ x: 0, opacity: 1 }}
onClick={() => toggleGroup(group.splitwiseId)}
+ role="button"
+ tabIndex={0}
+ aria-pressed={isSelected}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ toggleGroup(group.splitwiseId);
+ }
+ }}
className={`transition-all cursor-pointer p-4 ${isNeo🤖 Prompt for AI Agents
In `@web/pages/SplitwiseGroupSelection.tsx` around lines 178 - 195, The group card
motion.div is not keyboard-accessible; update the clickable element (the
motion.div rendered for each group using key={group.splitwiseId}) to be
focusable and operable via keyboard: either replace it with a semantic
interactive element (button or input type="checkbox") or add tabIndex={0},
onKeyDown handler that calls toggleGroup(group.splitwiseId) for Enter/Space, and
role="button" or role="checkbox" with aria-checked={isSelected} to expose
selection state; ensure visual focus styles remain and keep the existing onClick
behavior and Check rendering tied to isSelected.
Comment on lines
+15
to
+26
| const handleOAuthImport = async () => { | ||
| setLoading(true); | ||
| try { | ||
| const response = await getSplitwiseAuthUrl(); | ||
| const { authorization_url } = response.data; | ||
|
|
||
| // Redirect to Splitwise OAuth page | ||
| window.location.href = authorization_url; | ||
| } catch (error: any) { | ||
| console.error('OAuth error:', error); | ||
| addToast(error.response?.data?.detail || 'Failed to initiate authorization', 'error'); | ||
| setLoading(false); |
Contributor
There was a problem hiding this comment.
Guard against missing/invalid authorization_url before redirect.
If the backend response is malformed, the user can be redirected to undefined and remain stuck in loading. Add a quick check and reset state.
🛡️ Proposed fix
- const { authorization_url } = response.data;
-
- // Redirect to Splitwise OAuth page
- window.location.href = authorization_url;
+ const { authorization_url } = response.data;
+ if (!authorization_url) {
+ addToast('Authorization URL missing from response', 'error');
+ setLoading(false);
+ return;
+ }
+ // Redirect to Splitwise OAuth page
+ window.location.href = authorization_url;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleOAuthImport = async () => { | |
| setLoading(true); | |
| try { | |
| const response = await getSplitwiseAuthUrl(); | |
| const { authorization_url } = response.data; | |
| // Redirect to Splitwise OAuth page | |
| window.location.href = authorization_url; | |
| } catch (error: any) { | |
| console.error('OAuth error:', error); | |
| addToast(error.response?.data?.detail || 'Failed to initiate authorization', 'error'); | |
| setLoading(false); | |
| const handleOAuthImport = async () => { | |
| setLoading(true); | |
| try { | |
| const response = await getSplitwiseAuthUrl(); | |
| const { authorization_url } = response.data; | |
| if (!authorization_url) { | |
| addToast('Authorization URL missing from response', 'error'); | |
| setLoading(false); | |
| return; | |
| } | |
| // Redirect to Splitwise OAuth page | |
| window.location.href = authorization_url; | |
| } catch (error: any) { | |
| console.error('OAuth error:', error); | |
| addToast(error.response?.data?.detail || 'Failed to initiate authorization', 'error'); | |
| setLoading(false); |
🤖 Prompt for AI Agents
In `@web/pages/SplitwiseImport.tsx` around lines 15 - 26, In handleOAuthImport,
guard against a missing or invalid authorization_url from getSplitwiseAuthUrl():
after extracting authorization_url from response.data, check its
presence/truthiness before setting window.location.href; if it's falsy, call
addToast with a clear error message, console.error the response for debugging,
and call setLoading(false) to reset the UI instead of redirecting to undefined.
Ensure this check is applied inside the try block before the redirect so the
user isn't left stuck in loading when the backend returns a malformed response.
Comment on lines
+56
to
+64
| {/* Main Button */} | ||
| <button | ||
| onClick={handleOAuthImport} | ||
| disabled={loading} | ||
| className={`w-full py-4 px-6 flex items-center justify-center gap-3 transition-all ${isNeo | ||
| ? 'bg-blue-500 border-2 border-black text-white font-bold shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] rounded-none' | ||
| : 'bg-blue-600 hover:bg-blue-700 text-white font-semibold rounded-lg shadow-lg hover:shadow-xl' | ||
| } disabled:opacity-50 disabled:cursor-not-allowed mb-6`} | ||
| > |
Contributor
There was a problem hiding this comment.
Set an explicit button type.
Avoid accidental form submission when this component is rendered inside a <form>.
✅ Proposed fix
- <button
+ <button
+ type="button"
onClick={handleOAuthImport}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {/* Main Button */} | |
| <button | |
| onClick={handleOAuthImport} | |
| disabled={loading} | |
| className={`w-full py-4 px-6 flex items-center justify-center gap-3 transition-all ${isNeo | |
| ? 'bg-blue-500 border-2 border-black text-white font-bold shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] rounded-none' | |
| : 'bg-blue-600 hover:bg-blue-700 text-white font-semibold rounded-lg shadow-lg hover:shadow-xl' | |
| } disabled:opacity-50 disabled:cursor-not-allowed mb-6`} | |
| > | |
| {/* Main Button */} | |
| <button | |
| type="button" | |
| onClick={handleOAuthImport} | |
| disabled={loading} | |
| className={`w-full py-4 px-6 flex items-center justify-center gap-3 transition-all ${isNeo | |
| ? 'bg-blue-500 border-2 border-black text-white font-bold shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:shadow-[2px_2px_0px_0px_rgba(0,0,0,1)] hover:translate-x-[2px] hover:translate-y-[2px] rounded-none' | |
| : 'bg-blue-600 hover:bg-blue-700 text-white font-semibold rounded-lg shadow-lg hover:shadow-xl' | |
| } disabled:opacity-50 disabled:cursor-not-allowed mb-6`} | |
| > |
🧰 Tools
🪛 Biome (2.1.2)
[error] 57-64: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@web/pages/SplitwiseImport.tsx` around lines 56 - 64, The button currently
rendered in SplitwiseImport (the element invoking handleOAuthImport and using
props loading and isNeo) lacks an explicit type and can accidentally submit a
surrounding form; update the main button element to include type="button" to
prevent implicit form submission while preserving existing onClick, disabled,
and className behavior.
Comment on lines
+88
to
+112
| <div className={`w-5 h-5 flex items-center justify-center ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}> | ||
| <svg className="w-3 h-3 text-white" fill="currentColor" viewBox="0 0 20 20"> | ||
| <path fillRule="evenodd" d="M16.707 5.293a1 1 0 010 1.414l-8 8a1 1 0 01-1.414 0l-4-4a1 1 0 011.414-1.414L8 12.586l7.293-7.293a1 1 0 011.414 0z" clipRule="evenodd" /> | ||
| </svg> | ||
| </div> | ||
| What's being imported | ||
| </h3> | ||
| <ul className={`space-y-2 text-sm ${isNeo ? 'text-black/80' : 'text-gray-700 dark:text-gray-300'}`}> | ||
| {['All your friends', 'All your groups', 'All expenses & splits', 'All settlements'].map((item, idx) => ( | ||
| <li key={idx} className="flex items-center gap-2"> | ||
| <span className={`w-1.5 h-1.5 ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}></span> | ||
| <span>{item}</span> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
|
|
||
| {/* Important Notes */} | ||
| <div className={`${isNeo | ||
| ? 'bg-amber-50 border-2 border-black p-5 rounded-none shadow-[2px_2px_0px_0px_rgba(0,0,0,1)]' | ||
| : 'bg-amber-50 dark:bg-amber-900/10 border border-amber-200 dark:border-amber-800/50 rounded-lg p-5'}`}> | ||
| <h3 className={`font-bold text-base mb-3 flex items-center gap-2 ${isNeo ? 'text-black' : 'text-gray-900 dark:text-white'}`}> | ||
| <svg className={`w-5 h-5 ${isNeo ? 'text-black' : 'text-amber-600 dark:text-amber-500'}`} fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-3L13.732 4c-.77-1.333-2.694-1.333-3.464 0L3.34 16c-.77 1.333.192 3 1.732 3z" /> | ||
| </svg> |
Contributor
There was a problem hiding this comment.
Add accessibility metadata to inline SVG icons.
Biome flags missing SVG titles/labels. If these are decorative, mark them as hidden from assistive tech.
♿ Proposed fix
- <svg className="w-3 h-3 text-white" fill="currentColor" viewBox="0 0 20 20">
+ <svg
+ className="w-3 h-3 text-white"
+ fill="currentColor"
+ viewBox="0 0 20 20"
+ aria-hidden="true"
+ focusable="false"
+ >
<path fillRule="evenodd" d="M16.707 5.293a1 1 0 010 1.414l-8 8a1 1 0 01-1.414 0l-4-4a1 1 0 011.414-1.414L8 12.586l7.293-7.293a1 1 0 011.414 0z" clipRule="evenodd" />
</svg>
...
- <svg className={`w-5 h-5 ${isNeo ? 'text-black' : 'text-amber-600 dark:text-amber-500'}`} fill="none" stroke="currentColor" viewBox="0 0 24 24">
+ <svg
+ className={`w-5 h-5 ${isNeo ? 'text-black' : 'text-amber-600 dark:text-amber-500'}`}
+ fill="none"
+ stroke="currentColor"
+ viewBox="0 0 24 24"
+ aria-hidden="true"
+ focusable="false"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className={`w-5 h-5 flex items-center justify-center ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}> | |
| <svg className="w-3 h-3 text-white" fill="currentColor" viewBox="0 0 20 20"> | |
| <path fillRule="evenodd" d="M16.707 5.293a1 1 0 010 1.414l-8 8a1 1 0 01-1.414 0l-4-4a1 1 0 011.414-1.414L8 12.586l7.293-7.293a1 1 0 011.414 0z" clipRule="evenodd" /> | |
| </svg> | |
| </div> | |
| What's being imported | |
| </h3> | |
| <ul className={`space-y-2 text-sm ${isNeo ? 'text-black/80' : 'text-gray-700 dark:text-gray-300'}`}> | |
| {['All your friends', 'All your groups', 'All expenses & splits', 'All settlements'].map((item, idx) => ( | |
| <li key={idx} className="flex items-center gap-2"> | |
| <span className={`w-1.5 h-1.5 ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}></span> | |
| <span>{item}</span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| {/* Important Notes */} | |
| <div className={`${isNeo | |
| ? 'bg-amber-50 border-2 border-black p-5 rounded-none shadow-[2px_2px_0px_0px_rgba(0,0,0,1)]' | |
| : 'bg-amber-50 dark:bg-amber-900/10 border border-amber-200 dark:border-amber-800/50 rounded-lg p-5'}`}> | |
| <h3 className={`font-bold text-base mb-3 flex items-center gap-2 ${isNeo ? 'text-black' : 'text-gray-900 dark:text-white'}`}> | |
| <svg className={`w-5 h-5 ${isNeo ? 'text-black' : 'text-amber-600 dark:text-amber-500'}`} fill="none" stroke="currentColor" viewBox="0 0 24 24"> | |
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-3L13.732 4c-.77-1.333-2.694-1.333-3.464 0L3.34 16c-.77 1.333.192 3 1.732 3z" /> | |
| </svg> | |
| <div className={`w-5 h-5 flex items-center justify-center ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}> | |
| <svg | |
| className="w-3 h-3 text-white" | |
| fill="currentColor" | |
| viewBox="0 0 20 20" | |
| aria-hidden="true" | |
| focusable="false" | |
| > | |
| <path fillRule="evenodd" d="M16.707 5.293a1 1 0 010 1.414l-8 8a1 1 0 01-1.414 0l-4-4a1 1 0 011.414-1.414L8 12.586l7.293-7.293a1 1 0 011.414 0z" clipRule="evenodd" /> | |
| </svg> | |
| </div> | |
| What's being imported | |
| </h3> | |
| <ul className={`space-y-2 text-sm ${isNeo ? 'text-black/80' : 'text-gray-700 dark:text-gray-300'}`}> | |
| {['All your friends', 'All your groups', 'All expenses & splits', 'All settlements'].map((item, idx) => ( | |
| <li key={idx} className="flex items-center gap-2"> | |
| <span className={`w-1.5 h-1.5 ${isNeo ? 'bg-black rounded-none' : 'bg-blue-600 rounded-full'}`}></span> | |
| <span>{item}</span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| {/* Important Notes */} | |
| <div className={`${isNeo | |
| ? 'bg-amber-50 border-2 border-black p-5 rounded-none shadow-[2px_2px_0px_0px_rgba(0,0,0,1)]' | |
| : 'bg-amber-50 dark:bg-amber-900/10 border border-amber-200 dark:border-amber-800/50 rounded-lg p-5'}`}> | |
| <h3 className={`font-bold text-base mb-3 flex items-center gap-2 ${isNeo ? 'text-black' : 'text-gray-900 dark:text-white'}`}> | |
| <svg | |
| className={`w-5 h-5 ${isNeo ? 'text-black' : 'text-amber-600 dark:text-amber-500'}`} | |
| fill="none" | |
| stroke="currentColor" | |
| viewBox="0 0 24 24" | |
| aria-hidden="true" | |
| focusable="false" | |
| > | |
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-3L13.732 4c-.77-1.333-2.694-1.333-3.464 0L3.34 16c-.77 1.333.192 3 1.732 3z" /> | |
| </svg> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 89-89: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
[error] 110-110: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
🤖 Prompt for AI Agents
In `@web/pages/SplitwiseImport.tsx` around lines 88 - 112, The inline SVG icons in
SplitwiseImport.tsx (the checkmark SVG inside the first h3 and the warning
triangle SVG in the Important Notes h3) lack accessibility metadata; update each
SVG: if decorative, add aria-hidden="true" and focusable="false" (and remove
role), otherwise include a <title> element and aria-label/role="img" with
descriptive text; ensure both SVG elements are updated accordingly so screen
readers either ignore decorative icons or announce meaningful labels.
Comment on lines
+9
to
+11
| const isValidCurrencyCode = (code: string): code is CurrencyCode => { | ||
| return code in CURRENCIES; | ||
| }; |
Contributor
There was a problem hiding this comment.
Avoid prototype keys passing currency validation.
code in CURRENCIES accepts inherited keys (e.g., "toString"), which can lead to invalid currency.code and runtime errors. Prefer an own-property check.
✅ Safer validation
-const isValidCurrencyCode = (code: string): code is CurrencyCode => {
- return code in CURRENCIES;
-};
+const isValidCurrencyCode = (code: string): code is CurrencyCode => {
+ return Object.prototype.hasOwnProperty.call(CURRENCIES, code);
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isValidCurrencyCode = (code: string): code is CurrencyCode => { | |
| return code in CURRENCIES; | |
| }; | |
| const isValidCurrencyCode = (code: string): code is CurrencyCode => { | |
| return Object.prototype.hasOwnProperty.call(CURRENCIES, code); | |
| }; |
🤖 Prompt for AI Agents
In `@web/utils/formatters.ts` around lines 9 - 11, The current isValidCurrencyCode
function uses the "in" operator which returns true for inherited prototype keys
(e.g., "toString"); update the check to ensure only own properties of CURRENCIES
are accepted by replacing `code in CURRENCIES` with
`Object.prototype.hasOwnProperty.call(CURRENCIES, code)` (keep the function
signature `isValidCurrencyCode(code: string): code is CurrencyCode` so the type
guard remains).
Comment on lines
+58
to
+61
| try: | ||
| user_obj_id = ObjectId(user_id) | ||
| except: | ||
| user_obj_id = user_id |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/app/expenses/service.py | head -70Repository: Devasy/splitwiser
Length of output: 2726
🏁 Script executed:
# Check imports in the service file
head -20 backend/app/expenses/service.pyRepository: Devasy/splitwiser
Length of output: 543
🏁 Script executed:
cat -n backend/app/expenses/service.py | sed -n '195,205p;330,345p;775,790p;885,900p;945,960p'Repository: Devasy/splitwiser
Length of output: 3131
🏁 Script executed:
python3 << 'EOF'
from bson import errors
print("Available exceptions in bson.errors:")
print([attr for attr in dir(errors) if 'Invalid' in attr or 'Error' in attr])
EOFRepository: Devasy/splitwiser
Length of output: 177
Narrow bare except when parsing ObjectId to specific exception types.
Bare except masks unrelated errors. Change to except errors.InvalidId: (and optionally TypeError) to ensure unexpected failures surface correctly.
This pattern occurs at 6 locations and should be fixed consistently. Note that the file already uses the correct approach at lines 47-54, which can serve as a reference.
♻️ Suggested pattern
- try:
- user_obj_id = ObjectId(user_id)
- except:
- user_obj_id = user_id
+ try:
+ user_obj_id = ObjectId(user_id)
+ except errors.InvalidId:
+ user_obj_id = user_idAlso applies to: 197-200, 335-338, 780-783, 891-894, 951-954
🧰 Tools
🪛 Ruff (0.14.14)
60-60: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In `@backend/app/expenses/service.py` around lines 58 - 61, Replace the bare
except used when converting user_id to an ObjectId with a narrow exception
handler: catch bson.errors.InvalidId (and TypeError where appropriate) instead
of a naked except; specifically update the try/except around ObjectId(user_id)
that assigns user_obj_id so it becomes except (bson.errors.InvalidId,
TypeError): user_obj_id = user_id. Apply the same change consistently for all
occurrences that convert user_id (the six spots using ObjectId(user_id) which
currently use a bare except), and import bson.errors.InvalidId where needed.
Comment on lines
+230
to
+235
| # Add search filter | ||
| if search: | ||
| query["$or"] = [ | ||
| {"description": {"$regex": search, "$options": "i"}}, | ||
| {"tags": {"$regex": search, "$options": "i"}}, | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/app/expenses/service.py | sed -n '220,245p'Repository: Devasy/splitwiser
Length of output: 1072
🏁 Script executed:
cat -n backend/app/expenses/service.py | sed -n '1,230p' | grep -E "^\s+async def|^\s+def" | tail -5Repository: Devasy/splitwiser
Length of output: 43
🏁 Script executed:
cat -n backend/app/expenses/service.py | sed -n '180,240p'Repository: Devasy/splitwiser
Length of output: 2330
🏁 Script executed:
# Check if there's an API endpoint calling this function
rg "list_group_expenses" backend/app --type pyRepository: Devasy/splitwiser
Length of output: 280
🏁 Script executed:
cat -n backend/app/expenses/routes.py | grep -A 30 "async def list_group_expenses"Repository: Devasy/splitwiser
Length of output: 1350
🏁 Script executed:
head -30 backend/app/expenses/service.pyRepository: Devasy/splitwiser
Length of output: 746
🏁 Script executed:
# Check for index definitions in the codebase
rg "createIndex|create_index|ensureIndex" backend/app --type pyRepository: Devasy/splitwiser
Length of output: 43
🏁 Script executed:
# Look for database initialization or index setup
find backend -name "*.py" | xargs grep -l "index" | head -10Repository: Devasy/splitwiser
Length of output: 101
🏁 Script executed:
cat backend/migrations/001_create_indexes.pyRepository: Devasy/splitwiser
Length of output: 11191
🏁 Script executed:
# Search for other $regex uses in the codebase
rg "\$regex" backend/app --type py -B 2 -A 2Repository: Devasy/splitwiser
Length of output: 43
🏁 Script executed:
# Search for regex uses in the codebase
rg "regex" backend/app --type py -iRepository: Devasy/splitwiser
Length of output: 257
🏁 Script executed:
# Check if re module is imported in service.py
grep -n "^import\|^from" backend/app/expenses/service.py | head -20Repository: Devasy/splitwiser
Length of output: 376
🏁 Script executed:
# Check if there's any input validation on search parameter
cat -n backend/app/expenses/routes.py | sed -n '60,95p'Repository: Devasy/splitwiser
Length of output: 1577
Escape user-controlled search strings before $regex.
Using raw input in regex opens you to catastrophic backtracking and expensive scans. The search parameter comes directly from the HTTP request without validation and is passed to $regex without escaping.
🛡️ Proposed fix
+import re
...
- if search:
- query["$or"] = [
- {"description": {"$regex": search, "$options": "i"}},
- {"tags": {"$regex": search, "$options": "i"}},
- ]
+ if search:
+ safe = re.escape(search)
+ query["$or"] = [
+ {"description": {"$regex": safe, "$options": "i"}},
+ {"tags": {"$regex": safe, "$options": "i"}},
+ ]🤖 Prompt for AI Agents
In `@backend/app/expenses/service.py` around lines 230 - 235, The code in the
expenses search branch uses the raw search string directly in query["$or"] ->
{"$regex": search} which risks catastrophic backtracking and expensive DB scans;
update the logic that builds the query (the block that checks if search and sets
query["$or"]) to escape user-controlled input before using it in $regex (e.g.,
call a regex-escape helper or Python's re.escape on the search value), and also
enforce a sane length/character limit on search to prevent overly long patterns;
ensure the updated value is used for both description and tags regex entries.
Comment on lines
+587
to
591
| # Get all settlements for the group regardless of status | ||
| # We calculate net balances from ALL transactions to get true outstanding amounts | ||
| settlements = await self.settlements_collection.find( | ||
| {"groupId": group_id, "status": "pending"} | ||
| {"groupId": group_id} | ||
| ).to_list(None) |
Contributor
There was a problem hiding this comment.
Exclude cancelled settlements from balance calculations.
Including cancelled entries will inflate balances. Filter them out (or only include pending/completed explicitly).
🐛 Proposed fix
- settlements = await self.settlements_collection.find(
- {"groupId": group_id}
- ).to_list(None)
+ settlements = await self.settlements_collection.find(
+ {"groupId": group_id, "status": {"$ne": SettlementStatus.CANCELLED.value}}
+ ).to_list(None)Also applies to: 645-649
🤖 Prompt for AI Agents
In `@backend/app/expenses/service.py` around lines 587 - 591, The current query
fetching settlements (settlements = await
self.settlements_collection.find({"groupId": group_id}).to_list(None)) pulls
cancelled entries into balance calculations; update the query used in the
settlement fetches (the find call on self.settlements_collection used around the
balance calculation and similarly at the other occurrence) to exclude cancelled
settlements—e.g., add a filter for status != "cancelled" or explicitly include
only pending/completed statuses—so net balances are computed only from active
settlements.
Comment on lines
+1192
to
1194
| # Build group map for name lookup | ||
| groups_map = {str(g["_id"]): g.get("name", "Unknown Group") for g in groups} | ||
|
|
Contributor
There was a problem hiding this comment.
Remove unused groups_map.
groups_map is never used and should be deleted to avoid confusion.
🧹 Proposed fix
- groups_map = {str(g["_id"]): g.get("name", "Unknown Group") for g in groups}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Build group map for name lookup | |
| groups_map = {str(g["_id"]): g.get("name", "Unknown Group") for g in groups} | |
| # Build group map for name lookup | |
🧰 Tools
🪛 Ruff (0.14.14)
1193-1193: Local variable groups_map is assigned to but never used
Remove assignment to unused variable groups_map
(F841)
🤖 Prompt for AI Agents
In `@backend/app/expenses/service.py` around lines 1192 - 1194, Remove the unused
local variable groups_map (the dict comprehension building groups_map =
{str(g["_id"]): g.get("name", "Unknown Group") for g in groups}) from the
function—delete that line and any surrounding comment that solely documents it
so there are no unused variables left; ensure no other code (e.g., later
references to groups_map) depends on it and adjust those references if present.
Comment on lines
+1342
to
1347
| # Read from cached balances (will recalculate if missing) | ||
| cached = group.get("cachedBalances") | ||
| if cached is None: | ||
| # Cache miss - recalculate | ||
| cached = await self._recalculate_group_balances(group_id) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reuse _get_cached_balances to avoid duplicate cache-miss logic.
You already have a helper that recalculates on cache miss; using it here reduces drift and keeps the behavior consistent.
♻️ Proposed fix
- cached = group.get("cachedBalances")
- if cached is None:
- # Cache miss - recalculate
- cached = await self._recalculate_group_balances(group_id)
+ cached = await self._get_cached_balances(group_id)🤖 Prompt for AI Agents
In `@backend/app/expenses/service.py` around lines 1342 - 1347, Replace the manual
cache-check in the block that reads group.get("cachedBalances") with the
existing helper by calling await self._get_cached_balances(group_id) to obtain
cached balances; remove the duplicated cache-miss branch that calls
_recalculate_group_balances directly so the single helper (_get_cached_balances)
centralizes the cache retrieval-and-recalc logic and keeps behavior consistent.
Comment on lines
+205
to
+214
| const loadMoreExpenses = async () => { | ||
| if (!id || !hasMore || loadingMore) return; | ||
| setLoadingMore(true); | ||
| try { | ||
| const nextPage = page + 1; | ||
| const res = await getExpenses(id, nextPage); | ||
| setExpenses(prev => [...prev, ...res.data.expenses]); | ||
| setPage(nextPage); | ||
| setHasMore(res.data.pagination?.page < res.data.pagination?.totalPages); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
Load‑more ignores active search filter.
Line 210 fetches the next page without debouncedSearch, so results get mixed with unfiltered expenses when a search is active.
🛠️ Suggested fix
- const res = await getExpenses(id, nextPage);
+ const res = await getExpenses(id, nextPage, 20, debouncedSearch || undefined);🤖 Prompt for AI Agents
In `@web/pages/GroupDetails.tsx` around lines 205 - 214, The loadMoreExpenses
function ignores the active search by calling getExpenses(id, nextPage) without
the current debouncedSearch; update loadMoreExpenses to pass the search term
(debouncedSearch) into getExpenses (e.g., getExpenses(id, nextPage,
debouncedSearch)) so additional pages honor the active filter, and ensure state
updates (setExpenses, setPage, setHasMore) continue to operate on the filtered
result set returned by getExpenses.
Comment on lines
+41
to
+59
| const response = await axios.post(`${API_URL}/auth/refresh`, { | ||
| refresh_token: refreshToken | ||
| }); | ||
|
|
||
| const { access_token, refresh_token: newRefreshToken } = response.data; | ||
| localStorage.setItem('access_token', access_token); | ||
| if (newRefreshToken) { | ||
| localStorage.setItem('refresh_token', newRefreshToken); | ||
| } | ||
|
|
||
| // Retry the original request with new token | ||
| originalRequest.headers.Authorization = `Bearer ${access_token}`; | ||
| return api(originalRequest); | ||
| } catch (refreshError) { | ||
| // Refresh failed, redirect to login | ||
| localStorage.removeItem('access_token'); | ||
| localStorage.removeItem('refresh_token'); | ||
| window.location.href = '/login'; | ||
| return Promise.reject(refreshError); |
Contributor
There was a problem hiding this comment.
Access/refresh tokens in localStorage are XSS‑readable.
Lines 45–49 store tokens in localStorage, which is accessible to any injected script. Consider httpOnly cookies or in‑memory storage with a refresh cookie. At minimum, ensure strong XSS mitigations and short token lifetimes.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 45-45: Sensitive information detected in localStorage. Storing sensitive data like emails, usernames, or personal information in localStorage exposes it to malicious scripts and XSS attacks. Use secure storage alternatives or avoid storing sensitive data client-side.
Context: localStorage.setItem('access_token', access_token)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://cwe.mitre.org/data/definitions/312.html
- https://owasp.org/www-community/vulnerabilities/HTML5_Local_Storage_Security
(local-storage-sensitive-information)
🤖 Prompt for AI Agents
In `@web/services/api.ts` around lines 41 - 59, The code currently persists access
and refresh tokens to localStorage (lines around localStorage.setItem in the
axios refresh flow), which is XSS‑readable; instead, stop writing tokens to
localStorage and move to a cookie + memory approach: have the server issue a
Secure, HttpOnly refresh cookie and keep the short‑lived access token only in an
in‑memory store (e.g., an authService.get/setAccessToken backing variable) so
that in this file you set originalRequest.headers.Authorization = `Bearer
${accessTokenFromMemory}` and update that in-memory token when the refresh
response returns; remove localStorage.setItem/removeItem calls (and the
localStorage.removeItem calls on refresh failure) and instead trigger
logout/redirect while clearing in-memory token, and ensure the refresh POST does
not rely on client JS to set the refresh token (server cookie will be sent
automatically).
Comment on lines
+111
to
+113
| // Dashboard Analytics (all groups combined) | ||
| export const getDashboardAnalytics = async () => api.get('/users/me/friends-balance'); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate any dashboard analytics endpoints on the backend
rg -n "dashboard|analytics" backend/app -g '*.py' -C2Repository: Devasy/splitwiser
Length of output: 1789
🏁 Script executed:
# Examine the api.ts file to see the context and other endpoints
cat -n web/services/api.ts | head -150Repository: Devasy/splitwiser
Length of output: 7509
🏁 Script executed:
# Search for getFriendsBalance and other related endpoints
rg -n "getFriendsBalance|friends-balance|dashboard" web/services -C2Repository: Devasy/splitwiser
Length of output: 744
getDashboardAnalytics should call the correct analytics endpoint, not duplicate getFriendsBalance.
getDashboardAnalytics is currently pointing to /users/me/friends-balance (line 112), which is identical to the getFriendsBalance endpoint on line 100. The backend provides a proper analytics endpoint at /groups/{groupId}/analytics (see group_expense_analytics in backend/app/expenses/routes.py). For dashboard analytics combining all groups, either use the appropriate backend analytics endpoint or rename this function to match its actual behavior.
🤖 Prompt for AI Agents
In `@web/services/api.ts` around lines 111 - 113, getDashboardAnalytics currently
calls the friends-balance endpoint; change it to call the backend analytics
endpoint instead (or rename if the function should remain friends balance).
Update the exported function getDashboardAnalytics to accept an optional groupId
(or require groupId if backend needs it) and call `/groups/${groupId}/analytics`
(or `/groups/analytics` if your backend exposes an all-groups analytics route),
referencing the backend route group_expense_analytics; if the original function
was meant to return friends balance, rename it to getFriendsBalance to avoid
duplication.
Comment on lines
+115
to
+135
| export const getSplitwiseAuthUrl = async () => api.get('/import/splitwise/authorize'); | ||
| export const handleSplitwiseCallback = async (code?: string, state?: string, selectedGroupIds?: string[], accessToken?: string) => { | ||
| const payload: any = {}; | ||
|
|
||
| if (accessToken) { | ||
| payload.accessToken = accessToken; | ||
| } else if (code) { | ||
| payload.code = code; | ||
| payload.state = state || ''; | ||
| } | ||
|
|
||
| if (selectedGroupIds && selectedGroupIds.length > 0) { | ||
| payload.options = { selectedGroupIds }; | ||
| } | ||
|
|
||
| return api.post('/import/splitwise/callback', payload); | ||
| }; | ||
| export const getSplitwisePreview = async (accessToken: string) => api.post('/import/splitwise/preview', { access_token: accessToken }); | ||
| export const startSplitwiseImport = async (apiKey: string) => api.post('/import/splitwise/start', { api_key: apiKey }); | ||
| export const getImportStatus = async (importJobId: string) => api.get(`/import/status/${importJobId}`); | ||
| export const rollbackImport = async (importJobId: string) => api.post(`/import/rollback/${importJobId}`); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for preview route and api_key references in backend
rg -n "splitwise/preview|preview_splitwise" . -g '*.py' -C2Repository: Devasy/splitwiser
Length of output: 913
🏁 Script executed:
# Check StartImportRequest schema and api_key usage
rg -n "class StartImportRequest|api_key" . -g '*.py' -C3Repository: Devasy/splitwiser
Length of output: 10049
🏁 Script executed:
# Check backend router for splitwise endpoints
rg -n "splitwise|import" . -g '*.py' | grep -i "route\|endpoint\|@" | head -30Repository: Devasy/splitwiser
Length of output: 2242
🏁 Script executed:
# Get the full router file to see all endpoint definitions
cat -n ./backend/app/integrations/router.py | head -250Repository: Devasy/splitwiser
Length of output: 10566
🏁 Script executed:
# Check the StartImportRequest schema in detail
cat -n ./backend/app/integrations/schemas.py | sed -n '101,120p'Repository: Devasy/splitwiser
Length of output: 695
🏁 Script executed:
# Check for rollback endpoint and any other endpoints
cat -n ./backend/app/integrations/router.py | tail -100Repository: Devasy/splitwiser
Length of output: 4423
🏁 Script executed:
# Check the frontend API calls to verify exact payloads
cat -n ./web/services/api.ts | sed -n '115,135p'Repository: Devasy/splitwiser
Length of output: 1232
Remove getSplitwisePreview function and integrate preview into callback flow.
The endpoint /import/splitwise/preview does not exist in the backend. The preview functionality is actually handled within the /import/splitwise/callback endpoint—it returns preview data when no options.selectedGroupIds are provided. Remove the separate getSplitwisePreview call and instead use handleSplitwiseCallback without selectedGroupIds to get the preview.
Additionally, startSplitwiseImport sends { api_key } but the StartImportRequest schema only accepts provider and options fields. The backend retrieves API credentials from environment variables, not from the request payload. Update the frontend call to match the schema or reconsider the credential flow.
🤖 Prompt for AI Agents
In `@web/services/api.ts` around lines 115 - 135, Remove the unused
getSplitwisePreview function and any callers; for preview flows call
handleSplitwiseCallback(code?, state?) without passing selectedGroupIds so the
backend returns preview data (i.e., rely on handleSplitwiseCallback to POST to
/import/splitwise/callback with no payload.options to get preview). Also change
startSplitwiseImport to send a StartImportRequest-shaped payload (e.g., {
provider: 'splitwise', options: {/* if needed */} }) rather than { api_key },
since the backend expects provider/options and reads credentials from env
variables; update any callers of startSplitwiseImport accordingly.
Comment on lines
+84
to
+90
| export interface FriendBalance { | ||
| friendId: string; | ||
| userName: string; | ||
| netBalance: number; | ||
| owedToYou: number; | ||
| youOwe: number; | ||
| } |
Contributor
There was a problem hiding this comment.
FriendBalance shape diverges from backend response.
The backend FriendBalance model (backend/app/expenses/schemas.py, Lines 198–205) uses userId, owesYou, breakdown, and lastActivity, while this type uses friendId, owedToYou, and youOwe. Unless the API layer is mapping fields, this will cause runtime mismatches. Please align the type (or add an explicit adapter in the API layer).
🤖 Prompt for AI Agents
In `@web/types.ts` around lines 84 - 90, The FriendBalance TypeScript interface
currently declared as FriendBalance (fields friendId, userName, netBalance,
owedToYou, youOwe) does not match the backend model (userId, owesYou, breakdown,
lastActivity); update the interface to match the backend shape or add a mapping
layer in the API client that adapts the backend response to the existing
frontend shape. Specifically either (A) change the interface fields to userId,
owesYou, breakdown, lastActivity with appropriate types to mirror
backend/app/expenses/schemas.py, or (B) implement a transformer in the API call
that converts backend userId→friendId, owesYou→owedToYou/youOwe (and include
breakdown and lastActivity as needed) and return the current FriendBalance
shape; locate the FriendBalance interface in web/types.ts and the API response
handling code to perform the mapping.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Introduce integration for importing data from Splitwise, including groups, expenses, and settlements. Enhance the UI for consistency and add comprehensive validation and authentication services. Update backend routes and scripts to support the new functionality. Fix existing tests to ensure stability.
@coderabbitai review
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.