Split bill - #23
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates ORM models to SQLAlchemy 2.0 typed/dataclass models with cent-based money and Pydantic DTOs, updates services and routes to use DTOs and amount_cents, adds a Split Bill UI (TS/JS/CSS/templates) with frontend event wiring, and updates tests/end-to-end coverage. ChangesCore Infrastructure & Data Layer
Split Bill Feature
Test Coverage & Infrastructure
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
Dockerfile (1)
1-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the container as a non-root user.
No
USERis set, so both debug/prod containers run as root. That’s a security hardening gap (Line 1 through Line 21).🔧 Suggested hardening patch
FROM python:3.14-slim AS base +RUN addgroup --system app && adduser --system --ingroup app app WORKDIR /app COPY pyproject.toml . COPY README.md . ENV FLASK_APP=JustAnotherExpenseManager RUN pip install --upgrade pip COPY JustAnotherExpenseManager/ JustAnotherExpenseManager/ +RUN chown -R app:app /app FROM base AS debug RUN pip install -e ".[test,dev]" +USER app EXPOSE 5000 ENV JAEM_CONFIG=debug ENV FLASK_RUN_HOST=0.0.0.0 CMD ["flask", "run", "--debug"] FROM base AS prod RUN pip install -e . +USER app EXPOSE 5000 ENV JAEM_CONFIG=production ENV FLASK_RUN_HOST=0.0.0.0 CMD ["flask", "run"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 1 - 21, Create and use a non-root runtime user in the Dockerfile: add a dedicated user/group (for example "jaem") in the base image, chown the app files copied by COPY JustAnotherExpenseManager/ to that user (or set appropriate permissions), and set USER jaem in both the debug and prod stages so containers do not run as root; ensure the WORKDIR /app remains accessible to that user and any pip installs that must run as root are performed before switching to USER.JustAnotherExpenseManager/templates/transactions_list.html (1)
11-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
<th>for the split-select column causes full table misalignment when the toggle is active.The
<thead>has 6<th>cells. Each<tbody>row now has a 7th<td class="split-select-cell d-none">inserted as the first cell. Whiled-nonekeeps it hidden, once JavaScript removes that class, data rows have 7 visible columns against a 6-column header — the checkbox lands under "Date", "Date" data shifts under "Description", and "Action" overflows with no header.Add a matching hidden
<th>as the first header column, toggled in sync with the data cells:🐛 Proposed fix
<tr> + <th class="split-select-cell d-none"></th> <th>Date</th> <th>Description</th> <th>Type</th> <th>Category & Tags</th> <th>Amount</th> <th>Action</th> </tr>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/templates/transactions_list.html` around lines 11 - 28, Add a matching header cell for the hidden split-select column to keep columns aligned: insert a first <th> in the table header (e.g., with class "split-select-cell d-none" or similar) so it mirrors the tbody's <td class="split-select-cell d-none">; ensure both the <th> and <td> use the same CSS classes/selector (split-select-cell and d-none) so your existing JS toggle will show/hide the header and data cells in sync and prevent column shift.JustAnotherExpenseManager/static/js/transactions.js (1)
43-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
splitBillUpdateafterloadTransactions()finishes.Right now initial page load and every
loadTransactions()call from add/edit/delete/import can leave the split-bill total stale, and the fixed 100ms timeout on filter submit can still read the old table on a slow response. Trigger the event immediately after#transactions-listis replaced so the component always sees the actual visible rows.Suggested fix
async function loadTransactions(page) { page = page || 1; const params = new URLSearchParams(window.location.search); params.set("page", String(page)); const listEl = document.getElementById("transactions-list"); if (!listEl) return; try { listEl.innerHTML = await (await fetch("/api/transactions?" + params.toString())).text(); + emitSplitBillTotal(); } catch (error) { console.error("Error loading transactions:", error); listEl.innerHTML = "<p style=\"color: `#d63031`;\">Error loading transactions.</p>"; } } -document.querySelector("#filter-form")?.addEventListener("submit", () => { - setTimeout(emitSplitBillTotal, 100); -}); +document.querySelector("#filter-form")?.addEventListener("submit", () => { + // `loadTransactions()` now emits after the refreshed DOM is in place. +});Also applies to: 408-423
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/static/js/transactions.js` around lines 43 - 54, The transactions list replacement in loadTransactions currently doesn't notify the split-bill component, causing stale totals; after you set listEl.innerHTML in loadTransactions (and the similar replacement block around lines 408-423), dispatch a custom event named "splitBillUpdate" (or call element.dispatchEvent(new CustomEvent("splitBillUpdate"))) on the replaced `#transactions-list` element so the split-bill listener runs immediately once the DOM is updated; ensure this happens inside the try block right after innerHTML assignment and also after the fallback/error HTML assignment so both success and error paths emit the event.tests/test_models.py (3)
369-373:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline failure: Query filters don't match actual descriptions.
The queries filter by
description='First'anddescription='Second', but the transactions were created with descriptions'Test Transaction1'and'Test Transaction2'. This causestrans1andtrans2to beNone, leading toAttributeError: 'NoneType' object has no attribute 'tags'.Proposed fix
# Verify both transactions share the same tag - trans1 = db.query(Transaction).filter_by(description='First').first() - trans2 = db.query(Transaction).filter_by(description='Second').first() + trans1 = db.query(Transaction).filter_by(description='Test Transaction1').first() + trans2 = db.query(Transaction).filter_by(description='Test Transaction2').first()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 369 - 373, The test queries use the wrong description values so trans1/trans2 become None; update the query filters in tests/test_models.py where trans1 and trans2 are fetched (the db.query(Transaction).filter_by(...) calls) to match the actual created descriptions ("Test Transaction1" and "Test Transaction2") or else use a robust filter (e.g., filter by id or use an IN/LIKE on Transaction.description) so the fetched transactions are non-None before asserting on trans*.tags.
765-772:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline failure:
to_dict()returnsamount_cents, notamount.The test expects
d['amount']butTransaction.to_dict()returnsamount_centsas the key. Either the test assertion or the model'sto_dict()method needs updating for consistency.Proposed fix - update test to match model
- assert d['amount'] == 75.50 + assert d['amount_cents'] == 7550 # 75.50 dollars in centsOr update
to_dict()in the model to include'amount': self.amount_cents / 100.0if dollar representation is desired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 765 - 772, The test expects a dollar amount key 'amount' but Transaction.to_dict() currently emits 'amount_cents'; update the model's Transaction.to_dict() to include an 'amount' key (e.g., amount: self.amount_cents / 100.0, formatted as a float or rounded as project convention) in addition to or instead of 'amount_cents' so the saved.to_dict() call satisfies the assertions; modify the logic inside Transaction.to_dict() (the method named to_dict on the Transaction class) accordingly.
98-101:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline failure: repr assertion fails due to bug in model
__repr__.The test expects
'100.00'in repr but gets'$0.00'. This is caused by a bug inJustAnotherExpenseManager/models/__init__.pyline 172 whereamount_cents * 100.0should beamount_cents / 100.0. The fix belongs in the model file, not here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 98 - 101, The Transaction model's __repr__ uses the wrong arithmetic on amount_cents (it multiplies by 100.0), causing amounts to display as $0.00; in the Transaction.__repr__ (the method that builds repr_str) change the calculation to divide amount_cents by 100.0 (amount_cents / 100.0) and format the resulting value to two decimal places (so the string contains "100.00" for a 10000-cent amount) before embedding it in the repr.JustAnotherExpenseManager/utils/services.py (1)
87-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTags are added twice, causing duplicates.
Lines 87-96 create
tagListwith the category tag and pass it to theTransactionconstructor. Then lines 105-118 add the same category tag again viaadd_tag(). This results in duplicate tag associations or potential constraint violations.Remove either the initial tagList construction or the subsequent add_tag calls.
Proposed fix - remove duplicate tag addition
- tagList = [self._get_or_create_tag(row.category)] - if row.tags: - tagList = tagList + [self._get_or_create_tag(t) for t in row.tags] - transaction = Transaction( description=row.description, amount_cents=row.amount_cents, type=row.type, date=row.date, - tags=tagList + tags=[] ) # Add to session first so the object is tracked before any flush() # calls inside _get_or_create_tag — avoids the SAWarning about # "Object of type <Transaction> not in session". self.db.add(transaction) # Add category tag if provided if category: category_tag = self._get_or_create_tag(f'category:{category}') transaction.add_tag(category_tag)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/utils/services.py` around lines 87 - 118, The code currently builds tagList including the category tag and also calls transaction.add_tag(...) later, causing duplicate tags; fix by removing the initial inclusion of the category from tagList and instead only attach the category via the existing block that calls _get_or_create_tag(...) and transaction.add_tag(category_tag) (keep the initial tagList creation only for row.tags and the Transaction(...) call should receive just the tags derived from row.tags), ensuring you still call self.db.add(transaction) before any _get_or_create_tag calls; update references to tagList, Transaction(...), _get_or_create_tag, and transaction.add_tag accordingly.
🟠 Major comments (13)
Dockerfile-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign Docker Python version with declared support and CI test matrix.
Dockerfile uses Python 3.14-slim, but
pyproject.tomlclassifiers declare only 3.11 and 3.12 support, and all CI workflows test only 3.11. Either update the base image to a supported version (3.11 or 3.12) or extend project metadata and CI matrix to include 3.14.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 1, The Dockerfile currently uses "FROM python:3.14-slim" which is inconsistent with project metadata and CI; either change the Dockerfile's FROM to a supported runtime (e.g., "python:3.11-slim" or "python:3.12-slim") to match the classifiers in pyproject.toml and the CI matrix, or alternatively update pyproject.toml's "Programming Language :: Python :: 3.x" classifiers and the CI workflow matrix to include 3.14 so they all align; update the Dockerfile's FROM line or the pyproject.toml classifiers and CI job matrix (whichever approach you pick) so Dockerfile, pyproject.toml, and CI test matrix all reference the same Python versions.JustAnotherExpenseManager/static/js/stats.js-12914-12914 (1)
12914-12914:⚠️ Potential issue | 🟠 MajorFix event-name casing: use
splitBillUpdateinstead ofSplitBillUpdate.The emitter at stats.js uses
"SplitBillUpdate"(uppercase S), but the listener at shared-Dkp2Sup-.js:32 and the other emitter at transactions.js:403 both use"splitBillUpdate"(lowercase s). Event names are case-sensitive; this mismatch will silently prevent stats.js from triggering updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/static/js/stats.js` at line 12914, In stats.js update the event name string used in the window.dispatchEvent(CustomEvent(...)) call so it matches the listener/emitter convention: change "SplitBillUpdate" to "splitBillUpdate" where the code constructs and dispatches the CustomEvent (look for window.dispatchEvent and new CustomEvent in stats.js) so the event name is case-consistent with the listener in shared-Dkp2Sup-.js and the emitter in transactions.js.JustAnotherExpenseManager/static/js/stats.js-12911-12920 (1)
12911-12920:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsanitized HTML insertion is still present (Line 12911) and flagged by SDL tooling.
innerHTMLassignment from fetched content is a security risk and currently triggers OSSAR warnings. Also, the error path at Line 12920 can avoidinnerHTMLentirely.🔒 Suggested hardening
async function loadStats() { const container = document.getElementById("stats-container"); if (!container) return; try { - container.innerHTML = await (await fetch("/api/stats" + window.location.search)).text(); + const html = await (await fetch("/api/stats" + window.location.search)).text(); + // Use your project sanitizer here (e.g., Trusted Types policy or DOM sanitizer utility). + container.innerHTML = sanitizeHtml(html); refreshCharts(window.location.search.slice(1)); @@ } catch (error) { console.error("Error loading stats:", error); - container.innerHTML = "<p style=\"color: `#d63031`;\">Error loading statistics.</p>"; + const errorEl = document.createElement("p"); + errorEl.style.color = "#d63031"; + errorEl.textContent = "Error loading statistics."; + container.replaceChildren(errorEl); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/static/js/stats.js` around lines 12911 - 12920, The code assigns fetched HTML directly into container.innerHTML (from the fetch("/api/stats" + window.location.search) response) which is a security risk; change the flow to parse and sanitize the response before insertion: fetch the text, parse it with DOMParser (or sanitize via a library like DOMPurify) and then use container.replaceChildren(...) with the sanitized nodes instead of setting innerHTML; also avoid innerHTML in the catch path—use container.textContent or create a safe element (e.g., a paragraph node with textContent and style) so the error message is inserted without raw HTML; keep the existing refreshCharts(...) and the expenseElement query/dispatch logic intact.JustAnotherExpenseManager/templates/transactions_list.html-7-9 (1)
7-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate
id="split-select-toggle"when multiple months are in the DOM.This partial is rendered once per month. If more than one month's transactions are loaded simultaneously, the page will have multiple elements sharing the same ID, which is invalid HTML.
document.getElementById('split-select-toggle')returns only the first match, leaving all subsequent months' toggle buttons unwired.Use a class (e.g.,
class="split-select-toggle") or derive a unique ID fromcurrent_month(e.g.,id="split-select-toggle-{{ current_month }}").🐛 Proposed fix
- <button id="split-select-toggle" class="btn btn-outline-secondary btn-sm" type="button"> + <button class="split-select-toggle btn btn-outline-secondary btn-sm" type="button"> Select for split </button>And update the JS selector from
getElementById('split-select-toggle')toquerySelectorAll('.split-select-toggle').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/templates/transactions_list.html` around lines 7 - 9, The button uses a duplicated id "split-select-toggle" per-month; change the template to use either a class name like "split-select-toggle" or a unique id that includes the month (e.g., "split-select-toggle-{{ current_month }}") and then update the client JS that currently calls document.getElementById('split-select-toggle') to instead use document.querySelectorAll('.split-select-toggle') (or select by the new unique id pattern) and iterate to attach event listeners for each button; update references in any handlers that relied on the single-id to accept the element/context passed from the per-button listener.static_src/js/stats.ts-223-233 (1)
223-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
'SplitBillUpdate'to'splitBillUpdate'on line 226 of stats.ts.Line 226 dispatches
'SplitBillUpdate'(capital B), butsplit_bill.tsline 51 listens for'splitBillUpdate'(lowercase b). JavaScript event names are case-sensitive. The stats-page dispatch will silently drop — the split-bill component will never update when the summary page loads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static_src/js/stats.ts` around lines 223 - 233, The dispatched CustomEvent in stats.ts uses the wrong event name 'SplitBillUpdate'; update the CustomEvent in the block that builds and dispatches the event (where expenseElement is queried and window.dispatchEvent is called) to use the lowercase event name 'splitBillUpdate' so it matches the listener in split_bill.ts and the SplitBillUpdateEvent detail remains unchanged.static_src/js/split_bill.ts-112-119 (1)
112-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't rebuild the entire table on every percentage keystroke.
The
inputhandler callsupdatePercentage(), which immediately rerenderstableBody.innerHTML. That replaces the active<input>mid-edit, so multi-digit and decimal entry will lose caret/focus and feel broken. Either keep live updates in-place or defer the full rerender tochange/blur.Also applies to: 144-203, 298-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static_src/js/split_bill.ts` around lines 112 - 119, The handler updatePercentage currently forces a full table rerender (renderTotalAndTable) on every keystroke which replaces the active input and loses caret/focus; instead, stop doing a full DOM rebuild inside updatePercentage: update the model (person.percentage, person.locked) and update the specific input/display DOM nodes in-place (or update only totals) so the editing input isn't replaced, then call rebalanceUnlocked/savePeople but defer renderTotalAndTable to the input's change/blur event or use a short debounce for final render; apply the same fix pattern to the other affected handlers referenced around lines 144-203 and 298-302 so only targeted DOM updates or deferred full rerenders occur.tests/test_routes.py-43-52 (1)
43-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce setup for the seeded-data assertions.
These tests still assert on fixture-backed content (
Restaurant,Grocery shopping, non-empty categories), but the changed versions no longer requestsample_transactionsor create equivalent records inline. That leaves them running against an empty DB and matches the current CI failures.Also applies to: 130-156, 194-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes.py` around lines 43 - 52, The failing tests are asserting on seeded fixture data but no longer request the fixture, so update the test functions (e.g., test_get_transactions_page1_shows_newest_month and test_get_transactions_page2_shows_older_month and the other affected tests) to use the sample_transactions fixture or explicitly seed the DB before making the client.get call; specifically, add sample_transactions as a test parameter (def test_get_transactions_page1_shows_newest_month(self, client, sample_transactions): ...) or call the existing seed helper to create the Restaurant/Grocery shopping records and non-empty categories before the GET so the assertions run against populated data.static_src/js/split_bill.ts-28-39 (1)
28-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
splitBillPeoplebefore rendering it into HTML.
loadPeople()trustsJSON.parse()asPerson[], and the row template later injectsp.idandp.percentagestraight intoinnerHTMLattributes. A tamperedsessionStoragepayload can therefore become arbitrary markup/attributes on the next render. Coerce and validate the stored shape before assigningthis.people, or build the row DOM without string HTML.Also applies to: 166-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static_src/js/split_bill.ts` around lines 28 - 39, loadPeople currently assigns parsed JSON directly to this.people and trusts p.id and p.percentage are safe; validate/coerce each parsed item from sessionStorage (STORAGE_KEY) before assigning: ensure each entry has an id and percentage of the expected types (e.g., id as string/number, percentage as number within 0–100) and discard or default invalid entries. Additionally, change the rendering logic that uses string innerHTML (the row template that injects p.id and p.percentage) to build DOM nodes with createElement and set textContent/attributes safely or explicitly escape values, so no untrusted markup from sessionStorage can be injected. Ensure nextId calculation uses the validated this.people.tests/test_services.py-24-36 (1)
24-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet
_create()fail fast instead of returning a Flask response.Most callers use this helper as setup and ignore its return value, so converting
ValueErrorinto(jsonify(...), 400)hides broken fixture creation and produces the later "got 0/empty" failures you're seeing. Raise the exception here, or assert success inside the helper.Suggested fix
-def _create(service, description, amount, trans_type, date, category=None, tags=None) -> int | tuple[Response, int]: +def _create(service, description, amount, trans_type, date, category=None, tags=None) -> int: """Thin wrapper that accepts a plain string type for convenience.""" - try: - return service.create_transaction( - description=description, - amount_dollars=amount, - type=TransactionType(trans_type), - date=date, - category=category, - tags=tags or [], - ) - except ValueError as e: - return jsonify({'error': str(e)}), 400 + return service.create_transaction( + description=description, + amount_dollars=amount, + type=TransactionType(trans_type), + date=date, + category=category, + tags=tags or [], + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_services.py` around lines 24 - 36, The helper function _create currently catches ValueError from service.create_transaction and returns a Flask response, which hides setup failures; remove the try/except (or re-raise the caught ValueError) so the exception propagates to the test runner (or assert the return is a successful int), specifically update _create to call service.create_transaction(...) with TransactionType(trans_type) and allow any ValueError to bubble up instead of returning (jsonify(...), 400).JustAnotherExpenseManager/models/__init__.py-139-140 (1)
139-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
add_tagsilently fails whentagsis an empty list.The condition
if self.tagsevaluates toFalsefor an empty list[], preventing tags from being added to a transaction with no existing tags. Use explicitNonecheck instead.Proposed fix
def add_tag(self, tag: Tag) -> None: - if self.tags and tag not in self.tags: + if self.tags is not None and tag not in self.tags: self.tags.append(tag)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 139 - 140, In add_tag, the condition `if self.tags and tag not in self.tags` prevents adding tags when self.tags is an empty list; change the logic to treat None differently from an empty list: initialize self.tags to an empty list when it is None, then append only if tag is not already present (i.e., check `self.tags is None` then set `self.tags = []`, then `if tag not in self.tags: self.tags.append(tag)`) so add_tag will work for transactions with no existing tags.static_src/js/transactions.ts-510-522 (1)
510-522:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential index mismatch between rows and checkboxes.
The code iterates over
tr[data-amount]rows and accessescheckboxes[index]assuming 1:1 correspondence. If the DOM contains rows without checkboxes (or vice versa), this will access wrong checkboxes or throw errors whencheckboxes[index]is undefined.Consider iterating over checkboxes directly or querying the checkbox within each row:
Proposed fix
- document.querySelectorAll<HTMLElement>('tr[data-amount]').forEach((row, index) => { - const amount = parseFloat(row.dataset.amount ?? '0'); - if (isNaN(amount)) { - console.error('row: ' + index + ' amount is NAN'); - return; - } - if (checkboxes[index].checked) { - checked += amount; - } - else { - unchecked += amount; - } - }); + document.querySelectorAll<HTMLElement>('tr[data-amount]').forEach((row) => { + const amount = parseFloat(row.dataset.amount ?? '0'); + if (isNaN(amount)) { + console.error('row amount is NaN:', row); + return; + } + const checkbox = row.querySelector<HTMLInputElement>('.split-select-checkbox'); + if (checkbox?.checked) { + checked += amount; + } else { + unchecked += amount; + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static_src/js/transactions.ts` around lines 510 - 522, The loop over document.querySelectorAll<HTMLElement>('tr[data-amount]') uses checkboxes[index] which can mismatch; instead locate the checkbox for each row and guard it. For each row (row.dataset.amount) use row.querySelector<HTMLInputElement>('input[type="checkbox"]') (or fallback to closest checkbox) and skip if the checkbox is missing; parse the amount with parseFloat(row.dataset.amount ?? '0') as before and add to checked or unchecked based on the found checkbox.checked to avoid index misalignment or undefined access.JustAnotherExpenseManager/models/dtos.py-88-91 (1)
88-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
amount_centsdefault check is always true.Line 88 checks
if amount_cents is not None, butamount_centshas a default value of0, so this condition is alwaysTrue. This means even whenamount_dollarsis provided,amount_cents=0is also added to kwargs, potentially overriding the intended conversion.Proposed fix - check for explicit non-default value
def __init__( self, *, amount_dollars: Optional[float] = None, - amount_cents: int = 0, + amount_cents: Optional[int] = None, type: Optional[TransactionType] = None, type_str: Optional[str] = None, **kwargs: Unpack[TransactionKwargs]): - if amount_cents is not None: + if amount_cents is not None and amount_cents != 0: kwargs['amount_cents'] = amount_cents # type: ignore if amount_dollars is not None: kwargs['amount_dollars'] = amount_dollars # type: ignore🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/dtos.py` around lines 88 - 91, The code unconditionally adds amount_cents to kwargs because amount_cents defaults to 0, so change the guard in the DTO constructor (the block handling amount_cents and amount_dollars) to only add amount_cents when it was explicitly set to a non-default value (e.g., replace "if amount_cents is not None:" with a check like "if amount_cents != 0:" or another sentinel indicating presence) so that providing amount_dollars doesn't get overridden; apply the same presence-aware logic for amount_dollars if needed and update the kwargs assignments accordingly.JustAnotherExpenseManager/models/dtos.py-36-47 (1)
36-47:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe model_validator with
mode='after'is broken and the@model_validatorusage is deprecated.Lines 49-57 use
mode='after', which passes a model instance (not a dict) to the validator. Theisinstance(data, dict)check will always be False, so the validator does nothing. Additionally, using@model_validatorwithmode='after'on a classmethod is deprecated as of Pydantic v2.12 and should be rewritten as an instance method instead.The setters on
amount_dollarsandtype_str(lines 36-38, 45-47) are valid in Pydantic v2—computed fields do support setters for post-instantiation assignment.
🟡 Minor comments (4)
JustAnotherExpenseManager/static/css/styles.css-615-615 (1)
615-615:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename keyframes to kebab-case to satisfy stylelint.
Lines 618 and 643 violate the configured
keyframes-name-patternrule. Please rename both keyframes and their references on Lines 615 and 640.Proposed diff
-.modal { +.modal { display: none; @@ - animation: fadeIn 0.3s; + animation: fade-in 0.3s; } -@keyframes fadeIn { +@keyframes fade-in { @@ .modal-content { @@ - animation: slideDown 0.3s; + animation: slide-down 0.3s; } -@keyframes slideDown { +@keyframes slide-down {Also applies to: 618-618, 640-640, 643-643
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/static/css/styles.css` at line 615, Rename the camelCase keyframe identifiers to kebab-case and update their animation references: change the keyframe named "fadeIn" to "fade-in" and update the animation declaration (currently "animation: fadeIn 0.3s;") to "animation: fade-in 0.3s;", and likewise change the other keyframe (e.g., "slideUp") to "slide-up" and update its animation reference (the one around line 640) to "animation: slide-up ...;". Ensure both `@keyframes` blocks (the definitions currently named fadeIn and the other camelCase keyframe) are renamed to their kebab-case equivalents and all usages in the stylesheet match.JustAnotherExpenseManager/static/js/stats.js-12913-12917 (1)
12913-12917:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways dispatch the split-bill update, even when summary value is missing.
If
.summary-valueis absent, no event is emitted and downstream split-bill total can stay stale. Emit a fallback total of0consistently.✅ Suggested fix
- const expenseElement = container.querySelector(".summary-card.expense .summary-value"); - if (expenseElement) window.dispatchEvent(new CustomEvent("SplitBillUpdate", { detail: { - total: parseFloat(expenseElement.textContent.replace(/[$,]/g, "")) || 0, - source: "summary" - } })); + const expenseElement = container.querySelector(".summary-card.expense .summary-value"); + const total = parseFloat((expenseElement?.textContent ?? "").replace(/[$,]/g, "")) || 0; + window.dispatchEvent(new CustomEvent("SplitBillUpdate", { detail: { + total, + source: "summary" + } }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/static/js/stats.js` around lines 12913 - 12917, The current code only dispatches the "SplitBillUpdate" event when expenseElement exists, which leaves downstream consumers stale; always dispatch a CustomEvent named "SplitBillUpdate" from the same container context and use a fallback total of 0 when expenseElement is missing by computing total from parseFloat(expenseElement.textContent.replace(/[$,]/g, "")) || 0 if expenseElement exists otherwise 0, keeping the detail.source as "summary" and reusing the existing expenseElement, container, and "SplitBillUpdate" identifiers.static_src/js/stats.ts-228-228 (1)
228-228:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
textContentis typedstring | null— add a null guard.
Node.textContentreturnsstring | nullin the TypeScript DOM types. WithstrictNullChecks,.replace()on it is a type error. At runtime it is always a string for anHTMLElement, but the defensive fix is a one-liner:🛡️ Proposed fix
- total: parseFloat(expenseElement.textContent.replace(/[$,]/g, '')) || 0, + total: parseFloat((expenseElement.textContent ?? '').replace(/[$,]/g, '')) || 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static_src/js/stats.ts` at line 228, Guard against textContent being null before calling replace: update the expression that sets total (the parseFloat call using expenseElement.textContent) to coerce or default textContent to an empty string first (e.g. use the nullish coalescing or String() around expenseElement.textContent) and then call .replace(/[$,]/g, '') and parseFloat; keep the fallback || 0 as-is. This change affects the total assignment that references expenseElement and its textContent property.JustAnotherExpenseManager/models/__init__.py-149-150 (1)
149-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
remove_tagsilently fails whentagsis an empty list.Same issue as
add_tag- the conditionif self.tagsis falsy for empty lists.Proposed fix
def remove_tag(self, tag: Tag) -> None: - if self.tags and tag in self.tags: + if self.tags is not None and tag in self.tags: self.tags.remove(tag)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 149 - 150, The remove_tag method currently guards with "if self.tags and tag in self.tags", which is falsy for empty lists and prevents removal checks; change the condition to "if tag in self.tags" (or ensure self.tags is always a list and then use "if tag in self.tags") so membership is tested correctly even when tags is empty; update the remove_tag function to use that membership check (matching the intended behavior of add_tag).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 169-173: In Transaction.__repr__ the amount is converted
incorrectly by multiplying amount_cents by 100.0; change the f-string expression
in __repr__ on the Transaction class to divide amount_cents by 100.0
(amount_cents / 100.0) so cents are converted to dollars correctly and the
formatted string shows the right dollar amount.
In `@JustAnotherExpenseManager/models/dtos.py`:
- Around line 49-57: The model_validator trigger_computation is written for
pre-construction but uses mode='after' so it never receives the input dict;
change the decorator to `@model_validator`(mode='before') and update the function
signature/behaviour to accept and return the raw values mapping (preserving the
existing logic that looks for 'amount_dollars' and 'type_str' and converts them
to 'amount_cents' and 'type' using _parse_transaction_type) so the
transformations run on the input dict before model construction; keep the method
name trigger_computation and ensure it returns the modified dict (not a model
instance).
In `@JustAnotherExpenseManager/routes/transactions.py`:
- Around line 186-190: The date format used when calling
service.create_transaction in the transactions route is wrong:
entry.date.strftime('%Y-%M-%d') uses %M (minutes) instead of %m (month); update
the format to '%Y-%m-%d' in the call that passes date (the
service.create_transaction invocation) so the persisted month matches the parsed
input and avoids invalid dates.
- Around line 39-46: The helper is passing ORM Tag objects into
TransactionDTO.tags causing validation errors; instead, normalize each
transaction's tags before constructing TransactionDTOs by mapping each t.tags to
plain serializable dicts or Tag DTOs (e.g., use TagDTO or extract tag.id/name)
so TransactionDTO(tags=...) receives primitives/DTOs; update the list
comprehension in routes/transactions.py where TransactionDTO(...) is built to
replace t.tags with the normalized tag list (reference TransactionDTO and the
Tag objects on t.tags).
In `@JustAnotherExpenseManager/utils/services.py`:
- Line 356: The call to _apply_transaction_filters is using the wrong argument
order; update the invocation that currently passes (stmt, categories,
time_range, start_date, end_date, tags) so it matches the function signature:
call _apply_transaction_filters(stmt, categories, tags, time_range, start_date,
end_date) to ensure tags and time_range/end_date map correctly to the
parameters.
- Around line 280-284: The net value is being computed in cents while the
returned 'income' and 'expenses' are in dollars; update the net calculation in
the return dict so it uses dollars (e.g., compute net as (income - expenses)
divided by 100.0 or compute net from the already converted dollar values) and
then round to 2 decimals to match 'income' and 'expenses' (refer to the
'income', 'expenses', and 'net' keys and the income/expenses variables in the
return statement).
In `@tests/test_app.py`:
- Line 76: The test failure shows amount_cents is being stored/read as 0, so fix
the create/read mapping in the transaction path: inspect the API handler (e.g.
create_transaction or POST transaction endpoint), the service layer method that
constructs the Transaction model (e.g. TransactionService.create or
create_transaction), and the Transaction model/ORM field amount_cents or
serializer; ensure incoming decimal/float amounts are converted to integer cents
using a deterministic conversion (multiply by 100 and round to nearest int or
use integer arithmetic/Decimal quantize) before assigning to
Transaction.amount_cents, and ensure the read/serialization path returns that
integer (or converts back consistently) so saved.amount_cents == 9999 in tests;
also verify the DB column is integer and any migration code that previously
attempted cents migration is applied in the model/init path.
In `@tests/test_models.py`:
- Around line 25-31: The test constructs a Transaction with tags=None which
raises TypeError because the Transaction model expects a list-like for the tags
relationship; update the Transaction instantiation in tests/test_models.py (the
Transaction(...) call that sets tags) to pass an empty list (e.g., tags=[]) or
omit the tags argument so it defaults to an empty list, ensuring the Transaction
model receives a list-like value instead of None.
- Around line 123-125: The test fails because the ORM Transaction model exposes
amount_cents but not amount_dollars; either update the test to assert on
saved.amount_cents (e.g. 12346) or add a read-only property on the Transaction
model named amount_dollars that returns the dollar value computed from
amount_cents (rounded to two decimals) so tests can access saved.amount_dollars;
locate the Transaction class and implement a `@property` amount_dollars that
computes round(self.amount_cents / 100.0, 2) and update tests/test_models.py
accordingly if you choose the test-change approach.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 1-21: Create and use a non-root runtime user in the Dockerfile:
add a dedicated user/group (for example "jaem") in the base image, chown the app
files copied by COPY JustAnotherExpenseManager/ to that user (or set appropriate
permissions), and set USER jaem in both the debug and prod stages so containers
do not run as root; ensure the WORKDIR /app remains accessible to that user and
any pip installs that must run as root are performed before switching to USER.
In `@JustAnotherExpenseManager/static/js/transactions.js`:
- Around line 43-54: The transactions list replacement in loadTransactions
currently doesn't notify the split-bill component, causing stale totals; after
you set listEl.innerHTML in loadTransactions (and the similar replacement block
around lines 408-423), dispatch a custom event named "splitBillUpdate" (or call
element.dispatchEvent(new CustomEvent("splitBillUpdate"))) on the replaced
`#transactions-list` element so the split-bill listener runs immediately once the
DOM is updated; ensure this happens inside the try block right after innerHTML
assignment and also after the fallback/error HTML assignment so both success and
error paths emit the event.
In `@JustAnotherExpenseManager/templates/transactions_list.html`:
- Around line 11-28: Add a matching header cell for the hidden split-select
column to keep columns aligned: insert a first <th> in the table header (e.g.,
with class "split-select-cell d-none" or similar) so it mirrors the tbody's <td
class="split-select-cell d-none">; ensure both the <th> and <td> use the same
CSS classes/selector (split-select-cell and d-none) so your existing JS toggle
will show/hide the header and data cells in sync and prevent column shift.
In `@JustAnotherExpenseManager/utils/services.py`:
- Around line 87-118: The code currently builds tagList including the category
tag and also calls transaction.add_tag(...) later, causing duplicate tags; fix
by removing the initial inclusion of the category from tagList and instead only
attach the category via the existing block that calls _get_or_create_tag(...)
and transaction.add_tag(category_tag) (keep the initial tagList creation only
for row.tags and the Transaction(...) call should receive just the tags derived
from row.tags), ensuring you still call self.db.add(transaction) before any
_get_or_create_tag calls; update references to tagList, Transaction(...),
_get_or_create_tag, and transaction.add_tag accordingly.
In `@tests/test_models.py`:
- Around line 369-373: The test queries use the wrong description values so
trans1/trans2 become None; update the query filters in tests/test_models.py
where trans1 and trans2 are fetched (the db.query(Transaction).filter_by(...)
calls) to match the actual created descriptions ("Test Transaction1" and "Test
Transaction2") or else use a robust filter (e.g., filter by id or use an IN/LIKE
on Transaction.description) so the fetched transactions are non-None before
asserting on trans*.tags.
- Around line 765-772: The test expects a dollar amount key 'amount' but
Transaction.to_dict() currently emits 'amount_cents'; update the model's
Transaction.to_dict() to include an 'amount' key (e.g., amount:
self.amount_cents / 100.0, formatted as a float or rounded as project
convention) in addition to or instead of 'amount_cents' so the saved.to_dict()
call satisfies the assertions; modify the logic inside Transaction.to_dict()
(the method named to_dict on the Transaction class) accordingly.
- Around line 98-101: The Transaction model's __repr__ uses the wrong arithmetic
on amount_cents (it multiplies by 100.0), causing amounts to display as $0.00;
in the Transaction.__repr__ (the method that builds repr_str) change the
calculation to divide amount_cents by 100.0 (amount_cents / 100.0) and format
the resulting value to two decimal places (so the string contains "100.00" for a
10000-cent amount) before embedding it in the repr.
---
Major comments:
In `@Dockerfile`:
- Line 1: The Dockerfile currently uses "FROM python:3.14-slim" which is
inconsistent with project metadata and CI; either change the Dockerfile's FROM
to a supported runtime (e.g., "python:3.11-slim" or "python:3.12-slim") to match
the classifiers in pyproject.toml and the CI matrix, or alternatively update
pyproject.toml's "Programming Language :: Python :: 3.x" classifiers and the CI
workflow matrix to include 3.14 so they all align; update the Dockerfile's FROM
line or the pyproject.toml classifiers and CI job matrix (whichever approach you
pick) so Dockerfile, pyproject.toml, and CI test matrix all reference the same
Python versions.
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 139-140: In add_tag, the condition `if self.tags and tag not in
self.tags` prevents adding tags when self.tags is an empty list; change the
logic to treat None differently from an empty list: initialize self.tags to an
empty list when it is None, then append only if tag is not already present
(i.e., check `self.tags is None` then set `self.tags = []`, then `if tag not in
self.tags: self.tags.append(tag)`) so add_tag will work for transactions with no
existing tags.
In `@JustAnotherExpenseManager/models/dtos.py`:
- Around line 88-91: The code unconditionally adds amount_cents to kwargs
because amount_cents defaults to 0, so change the guard in the DTO constructor
(the block handling amount_cents and amount_dollars) to only add amount_cents
when it was explicitly set to a non-default value (e.g., replace "if
amount_cents is not None:" with a check like "if amount_cents != 0:" or another
sentinel indicating presence) so that providing amount_dollars doesn't get
overridden; apply the same presence-aware logic for amount_dollars if needed and
update the kwargs assignments accordingly.
In `@JustAnotherExpenseManager/static/js/stats.js`:
- Line 12914: In stats.js update the event name string used in the
window.dispatchEvent(CustomEvent(...)) call so it matches the listener/emitter
convention: change "SplitBillUpdate" to "splitBillUpdate" where the code
constructs and dispatches the CustomEvent (look for window.dispatchEvent and new
CustomEvent in stats.js) so the event name is case-consistent with the listener
in shared-Dkp2Sup-.js and the emitter in transactions.js.
- Around line 12911-12920: The code assigns fetched HTML directly into
container.innerHTML (from the fetch("/api/stats" + window.location.search)
response) which is a security risk; change the flow to parse and sanitize the
response before insertion: fetch the text, parse it with DOMParser (or sanitize
via a library like DOMPurify) and then use container.replaceChildren(...) with
the sanitized nodes instead of setting innerHTML; also avoid innerHTML in the
catch path—use container.textContent or create a safe element (e.g., a paragraph
node with textContent and style) so the error message is inserted without raw
HTML; keep the existing refreshCharts(...) and the expenseElement query/dispatch
logic intact.
In `@JustAnotherExpenseManager/templates/transactions_list.html`:
- Around line 7-9: The button uses a duplicated id "split-select-toggle"
per-month; change the template to use either a class name like
"split-select-toggle" or a unique id that includes the month (e.g.,
"split-select-toggle-{{ current_month }}") and then update the client JS that
currently calls document.getElementById('split-select-toggle') to instead use
document.querySelectorAll('.split-select-toggle') (or select by the new unique
id pattern) and iterate to attach event listeners for each button; update
references in any handlers that relied on the single-id to accept the
element/context passed from the per-button listener.
In `@static_src/js/split_bill.ts`:
- Around line 112-119: The handler updatePercentage currently forces a full
table rerender (renderTotalAndTable) on every keystroke which replaces the
active input and loses caret/focus; instead, stop doing a full DOM rebuild
inside updatePercentage: update the model (person.percentage, person.locked) and
update the specific input/display DOM nodes in-place (or update only totals) so
the editing input isn't replaced, then call rebalanceUnlocked/savePeople but
defer renderTotalAndTable to the input's change/blur event or use a short
debounce for final render; apply the same fix pattern to the other affected
handlers referenced around lines 144-203 and 298-302 so only targeted DOM
updates or deferred full rerenders occur.
- Around line 28-39: loadPeople currently assigns parsed JSON directly to
this.people and trusts p.id and p.percentage are safe; validate/coerce each
parsed item from sessionStorage (STORAGE_KEY) before assigning: ensure each
entry has an id and percentage of the expected types (e.g., id as string/number,
percentage as number within 0–100) and discard or default invalid entries.
Additionally, change the rendering logic that uses string innerHTML (the row
template that injects p.id and p.percentage) to build DOM nodes with
createElement and set textContent/attributes safely or explicitly escape values,
so no untrusted markup from sessionStorage can be injected. Ensure nextId
calculation uses the validated this.people.
In `@static_src/js/stats.ts`:
- Around line 223-233: The dispatched CustomEvent in stats.ts uses the wrong
event name 'SplitBillUpdate'; update the CustomEvent in the block that builds
and dispatches the event (where expenseElement is queried and
window.dispatchEvent is called) to use the lowercase event name
'splitBillUpdate' so it matches the listener in split_bill.ts and the
SplitBillUpdateEvent detail remains unchanged.
In `@static_src/js/transactions.ts`:
- Around line 510-522: The loop over
document.querySelectorAll<HTMLElement>('tr[data-amount]') uses checkboxes[index]
which can mismatch; instead locate the checkbox for each row and guard it. For
each row (row.dataset.amount) use
row.querySelector<HTMLInputElement>('input[type="checkbox"]') (or fallback to
closest checkbox) and skip if the checkbox is missing; parse the amount with
parseFloat(row.dataset.amount ?? '0') as before and add to checked or unchecked
based on the found checkbox.checked to avoid index misalignment or undefined
access.
In `@tests/test_routes.py`:
- Around line 43-52: The failing tests are asserting on seeded fixture data but
no longer request the fixture, so update the test functions (e.g.,
test_get_transactions_page1_shows_newest_month and
test_get_transactions_page2_shows_older_month and the other affected tests) to
use the sample_transactions fixture or explicitly seed the DB before making the
client.get call; specifically, add sample_transactions as a test parameter (def
test_get_transactions_page1_shows_newest_month(self, client,
sample_transactions): ...) or call the existing seed helper to create the
Restaurant/Grocery shopping records and non-empty categories before the GET so
the assertions run against populated data.
In `@tests/test_services.py`:
- Around line 24-36: The helper function _create currently catches ValueError
from service.create_transaction and returns a Flask response, which hides setup
failures; remove the try/except (or re-raise the caught ValueError) so the
exception propagates to the test runner (or assert the return is a successful
int), specifically update _create to call service.create_transaction(...) with
TransactionType(trans_type) and allow any ValueError to bubble up instead of
returning (jsonify(...), 400).
---
Minor comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 149-150: The remove_tag method currently guards with "if self.tags
and tag in self.tags", which is falsy for empty lists and prevents removal
checks; change the condition to "if tag in self.tags" (or ensure self.tags is
always a list and then use "if tag in self.tags") so membership is tested
correctly even when tags is empty; update the remove_tag function to use that
membership check (matching the intended behavior of add_tag).
In `@JustAnotherExpenseManager/static/css/styles.css`:
- Line 615: Rename the camelCase keyframe identifiers to kebab-case and update
their animation references: change the keyframe named "fadeIn" to "fade-in" and
update the animation declaration (currently "animation: fadeIn 0.3s;") to
"animation: fade-in 0.3s;", and likewise change the other keyframe (e.g.,
"slideUp") to "slide-up" and update its animation reference (the one around line
640) to "animation: slide-up ...;". Ensure both `@keyframes` blocks (the
definitions currently named fadeIn and the other camelCase keyframe) are renamed
to their kebab-case equivalents and all usages in the stylesheet match.
In `@JustAnotherExpenseManager/static/js/stats.js`:
- Around line 12913-12917: The current code only dispatches the
"SplitBillUpdate" event when expenseElement exists, which leaves downstream
consumers stale; always dispatch a CustomEvent named "SplitBillUpdate" from the
same container context and use a fallback total of 0 when expenseElement is
missing by computing total from
parseFloat(expenseElement.textContent.replace(/[$,]/g, "")) || 0 if
expenseElement exists otherwise 0, keeping the detail.source as "summary" and
reusing the existing expenseElement, container, and "SplitBillUpdate"
identifiers.
In `@static_src/js/stats.ts`:
- Line 228: Guard against textContent being null before calling replace: update
the expression that sets total (the parseFloat call using
expenseElement.textContent) to coerce or default textContent to an empty string
first (e.g. use the nullish coalescing or String() around
expenseElement.textContent) and then call .replace(/[$,]/g, '') and parseFloat;
keep the fallback || 0 as-is. This change affects the total assignment that
references expenseElement and its textContent property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7effe85e-4d5a-42ec-8517-5218dc0a0e4d
📒 Files selected for processing (35)
DockerfileJustAnotherExpenseManager/models/__init__.pyJustAnotherExpenseManager/models/dtos.pyJustAnotherExpenseManager/routes/stats.pyJustAnotherExpenseManager/routes/transactions.pyJustAnotherExpenseManager/static/css/split_bill.cssJustAnotherExpenseManager/static/css/styles.cssJustAnotherExpenseManager/static/js/shared-Dkp2Sup-.jsJustAnotherExpenseManager/static/js/stats.jsJustAnotherExpenseManager/static/js/transactions.jsJustAnotherExpenseManager/templates/split_bill_component.htmlJustAnotherExpenseManager/templates/summary.htmlJustAnotherExpenseManager/templates/transactions.htmlJustAnotherExpenseManager/templates/transactions_list.htmlJustAnotherExpenseManager/utils/database.pyJustAnotherExpenseManager/utils/services.pypyproject.tomlstatic_src/js/split_bill.tsstatic_src/js/stats.tsstatic_src/js/transactions.tsstatic_src/js/types.tstests/03-csv-import.spec.tstests/04-filters-stats.spec.tstests/06-security.spec.tstests/07-filter-combinations.spec.tstests/09-monthly-totals.spec.tstests/11-split-bill.spec.tstests/helpers.tstests/pages/SplitBillComponent.tstests/pages/TransactionsPage.tstests/test_app.pytests/test_integration.pytests/test_models.pytests/test_routes.pytests/test_services.py
💤 Files with no reviewable changes (1)
- tests/test_integration.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
JustAnotherExpenseManager/utils/services.py (1)
87-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate and incorrect tag creation logic.
There are two tag-creation paths that conflict:
Lines 87-96: Creates a tag with raw
row.category(e.g.,"food") and passes it directly to the Transaction constructor. This creates an incorrectly-named tag.Lines 104-118: The correct approach - creates
"category:food"tag and adds viaadd_tag().The first path creates a malformed tag and the second path duplicates work. Remove lines 87-96 or consolidate the logic.
Proposed fix: Remove duplicate logic, rely on existing correct path
def create_transaction( self, description: str, amount_dollars: float, type: TransactionType, date: str, category: str, tags: Optional[List[str]] = None ) -> int: """Create a new transaction.""" row = TransactionDTO( amount_dollars=amount_dollars, description=description, type=type, date=datetime.strptime(date, DT_FORMAT), category=category, tags=tags ) - tagList = [self._get_or_create_tag(row.category)] - if row.tags: - tagList = tagList + [self._get_or_create_tag(t) for t in row.tags] - transaction = Transaction( description=row.description, amount_cents=row.amount_cents, type=row.type, date=row.date, - tags=tagList + tags=[] )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/utils/services.py` around lines 87 - 118, Remove the duplicate/incorrect tag creation that builds tagList from row.category and row.tags before constructing Transaction; instead construct Transaction without pre-populated tags and rely on _get_or_create_tag + transaction.add_tag() calls that follow. Concretely, delete the block that creates tagList and passes tags=tagList into the Transaction constructor (the code invoking _get_or_create_tag for row.category and list-comprehension for row.tags), create the Transaction with only description/amount_cents/type/date, keep the self.db.add(transaction) call, and then use the existing category/tag handling that calls _get_or_create_tag(...) and transaction.add_tag(...) to add properly-named tags (including the "category:..." prefix) to avoid malformed/duplicated tags.
♻️ Duplicate comments (1)
JustAnotherExpenseManager/routes/transactions.py (1)
39-46:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNormalize tag objects to strings before building
TransactionDTO.Line 45 passes
TagORM objects directly intoTransactionDTO.tags, which expects strings. This causes the ValidationError shown in pipeline failures.Proposed fix
result['transactions'] = [TransactionDTO( description=t.description, amount_cents=t.amount_cents, category=t.category, date=t.date, type=t.type, - tags=[tag for tag in t.tags] if t.tags else [] + tags=[tag.name for tag in t.tags] if t.tags else [] ).model_dump() for t in result['transactions']]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/routes/transactions.py` around lines 39 - 46, The TransactionDTO is being constructed with ORM Tag objects (t.tags) instead of strings, causing validation errors; update the list comprehension inside the result['transactions'] conversion so that for each transaction t you pass tags as a list of strings (e.g., [tag.name for tag in t.tags] or str(tag) if appropriate) and keep the empty-list fallback (tags=[... for tag in t.tags] if t.tags else []) when calling TransactionDTO.model_dump(); ensure you reference the TransactionDTO construction and the t.tags usage in that list comprehension.
🧹 Nitpick comments (4)
JustAnotherExpenseManager/models/dtos.py (1)
36-47: 💤 Low valueSetters on
computed_fieldproperties are effectively dead code.Pydantic's
computed_fieldcreates read-only computed properties. The setters at lines 36-38 and 45-47 won't be invoked during model instantiation. The actual conversion is handled by themodel_validator(mode='before')at lines 49-57.Consider removing these setters to avoid confusion, or document that they're only usable for post-construction mutation (if that's intended).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/dtos.py` around lines 36 - 47, The setters attached to computed_field properties (amount_dollars.setter and type_str.setter) are dead for model creation because computed_field makes those properties read-only and the actual conversions are performed in model_validator(mode='before'); remove the amount_dollars.setter and type_str.setter to avoid confusion (or, if post-construction mutation is intended, add a comment above the computed_field properties clarifying they only work for attribute assignment after instantiation), and ensure all input conversion logic remains in the model_validator(mode='before') method.JustAnotherExpenseManager/utils/services.py (1)
11-11: ⚡ Quick winRemove unused import
RowDTO.Static analysis indicates
RowDTOis imported but never used.Proposed fix
-from JustAnotherExpenseManager.models.dtos import TransactionDTO, RowDTO +from JustAnotherExpenseManager.models.dtos import TransactionDTO🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/utils/services.py` at line 11, The import list in utils/services.py includes an unused symbol RowDTO from JustAnotherExpenseManager.models.dtos; remove RowDTO from the import statement so only TransactionDTO is imported (i.e., change the import line that references TransactionDTO and RowDTO to import just TransactionDTO) to eliminate the unused import warning and keep the module clean.JustAnotherExpenseManager/routes/transactions.py (1)
177-177: 💤 Low valueRemove commented-out dead code.
This line appears to be leftover from refactoring and serves no purpose.
Proposed fix
- `#trans_type` = _parse_transaction_type(entry.type)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/routes/transactions.py` at line 177, Remove the dead commented-out line referencing _parse_transaction_type in the transaction handling code: delete the commented line "#trans_type = _parse_transaction_type(entry.type)" from routes/transactions.py so the codebase has no leftover commented code; this affects the transaction parsing block where entry.type and the helper _parse_transaction_type are referenced.JustAnotherExpenseManager/models/__init__.py (1)
149-150: 💤 Low valueConsider making
remove_tagconsistent withadd_tagfix.If
add_tagis updated to initializeself.tagswhenNone,remove_tagshould remain safe (can't remove from empty list). However, consider whether aNonecheck should raise or log a warning for debugging purposes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 149 - 150, The remove_tag method currently assumes self.tags is iterable; make it consistent with add_tag by handling the None case explicitly: either initialize self.tags to an empty list if None (matching add_tag’s behavior) before attempting removal, or early-return/log a warning when self.tags is None to avoid silent failures; update the remove_tag implementation (referencing remove_tag, add_tag, and self.tags) to perform the chosen None check and a safe remove (e.g., check membership before remove) so removing a tag never raises when tags are uninitialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 139-140: The add_tag method currently checks "if self.tags and tag
not in self.tags" so it silently no-ops when self.tags is None; update add_tag
to ensure self.tags is initialized to an empty list when None (e.g., if
self.tags is None: self.tags = []), then append the tag only if tag not in
self.tags—this preserves idempotency and avoids silent failures; reference the
add_tag method and the self.tags attribute when making the change.
In `@JustAnotherExpenseManager/routes/transactions.py`:
- Around line 152-161: get_transactions is converting TransactionDTO instances
to dicts via .model_dump() before calling _render_transactions_list, which
causes _compute_month_totals (used inside _render_transactions_list) to fail
when it expects Transaction objects with attributes like is_income and
amount_cents; fix by stopping the conversion here — build the list as
TransactionDTO(...) objects (keep tags mapping as before) and pass that list to
_render_transactions_list so _compute_month_totals receives objects it can
access, or alternatively call _compute_month_totals before any .model_dump()
conversion if you prefer preserving dict output later; update the list
comprehension in get_transactions (the TransactionDTO construction) accordingly
and remove .model_dump() usage here so _render_transactions_list and
_compute_month_totals operate on DTO instances.
---
Outside diff comments:
In `@JustAnotherExpenseManager/utils/services.py`:
- Around line 87-118: Remove the duplicate/incorrect tag creation that builds
tagList from row.category and row.tags before constructing Transaction; instead
construct Transaction without pre-populated tags and rely on _get_or_create_tag
+ transaction.add_tag() calls that follow. Concretely, delete the block that
creates tagList and passes tags=tagList into the Transaction constructor (the
code invoking _get_or_create_tag for row.category and list-comprehension for
row.tags), create the Transaction with only description/amount_cents/type/date,
keep the self.db.add(transaction) call, and then use the existing category/tag
handling that calls _get_or_create_tag(...) and transaction.add_tag(...) to add
properly-named tags (including the "category:..." prefix) to avoid
malformed/duplicated tags.
---
Duplicate comments:
In `@JustAnotherExpenseManager/routes/transactions.py`:
- Around line 39-46: The TransactionDTO is being constructed with ORM Tag
objects (t.tags) instead of strings, causing validation errors; update the list
comprehension inside the result['transactions'] conversion so that for each
transaction t you pass tags as a list of strings (e.g., [tag.name for tag in
t.tags] or str(tag) if appropriate) and keep the empty-list fallback (tags=[...
for tag in t.tags] if t.tags else []) when calling TransactionDTO.model_dump();
ensure you reference the TransactionDTO construction and the t.tags usage in
that list comprehension.
---
Nitpick comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 149-150: The remove_tag method currently assumes self.tags is
iterable; make it consistent with add_tag by handling the None case explicitly:
either initialize self.tags to an empty list if None (matching add_tag’s
behavior) before attempting removal, or early-return/log a warning when
self.tags is None to avoid silent failures; update the remove_tag implementation
(referencing remove_tag, add_tag, and self.tags) to perform the chosen None
check and a safe remove (e.g., check membership before remove) so removing a tag
never raises when tags are uninitialized.
In `@JustAnotherExpenseManager/models/dtos.py`:
- Around line 36-47: The setters attached to computed_field properties
(amount_dollars.setter and type_str.setter) are dead for model creation because
computed_field makes those properties read-only and the actual conversions are
performed in model_validator(mode='before'); remove the amount_dollars.setter
and type_str.setter to avoid confusion (or, if post-construction mutation is
intended, add a comment above the computed_field properties clarifying they only
work for attribute assignment after instantiation), and ensure all input
conversion logic remains in the model_validator(mode='before') method.
In `@JustAnotherExpenseManager/routes/transactions.py`:
- Line 177: Remove the dead commented-out line referencing
_parse_transaction_type in the transaction handling code: delete the commented
line "#trans_type = _parse_transaction_type(entry.type)" from
routes/transactions.py so the codebase has no leftover commented code; this
affects the transaction parsing block where entry.type and the helper
_parse_transaction_type are referenced.
In `@JustAnotherExpenseManager/utils/services.py`:
- Line 11: The import list in utils/services.py includes an unused symbol RowDTO
from JustAnotherExpenseManager.models.dtos; remove RowDTO from the import
statement so only TransactionDTO is imported (i.e., change the import line that
references TransactionDTO and RowDTO to import just TransactionDTO) to eliminate
the unused import warning and keep the module clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0b3d74b-e294-4448-b7e8-a719efd7aad7
📒 Files selected for processing (6)
JustAnotherExpenseManager/models/__init__.pyJustAnotherExpenseManager/models/dtos.pyJustAnotherExpenseManager/routes/transactions.pyJustAnotherExpenseManager/utils/services.pytests/test_app.pytests/test_models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_app.py
- tests/test_models.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
JustAnotherExpenseManager/models/__init__.py (1)
139-143:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
add_tagdrops the first tag whenself.tagsisNone.If
self.tagsisNone, Line 141-142 initializes an empty list but never appendstag, causing silent data loss.Suggested fix
def add_tag(self, tag: Tag) -> None: @@ - if self.tags and tag not in self.tags: - self.tags.append(tag) - elif self.tags is None: - self.tags = [] + if self.tags is None: + self.tags = [] + if tag not in self.tags: + self.tags.append(tag)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 139 - 143, The add_tag method currently initializes self.tags to an empty list when it's None but never appends the provided tag, dropping the first tag; change the logic in add_tag (referencing add_tag, self.tags, and tag) so that when self.tags is None you create a list containing the tag (or set self.tags = [] and then append tag), and otherwise only append tag if it's not already in self.tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_app.py`:
- Around line 44-46: The assertion is checking the first render (templates[0])
instead of the GET response; update the test to assert against the retrieval
render by using the last template render (templates[-1]) after calling
client.get('/api/transactions?page=1'), so the line that reads something like
"_, context = templates[0]" should be changed to "_, context = templates[-1]" to
validate context['transactions'][0]['description'] == 'Lifecycle Expense'.
In `@tests/test_integration.py`:
- Line 49: Update the inline commented assertions in tests/test_integration.py
to satisfy flake8 E265 by adding a single space after each '#' in the commented
lines (for example change "`#assert` b'Updated Transaction'..." to "# assert
b'Updated Transaction'..." and do the same for the other commented line around
the original line 54); locate the comments by searching for the commented
assertion text "assert b'Updated Transaction'" and the other similar commented
assert and insert the space after the '#' to fix formatting.
- Around line 47-53: The test is asserting against the first render
(templates[0]) instead of the latest render after the PUT/DELETE; update the
assertions to use the most recent captured render (e.g., templates[-1]) when
checking edited and deleted transaction state so the checks after
client.put(...) and client.delete(...) reference the latest context (context =
templates[-1][1] or adjust the unpacking accordingly) and keep using trans_id
for the delete call.
---
Duplicate comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 139-143: The add_tag method currently initializes self.tags to an
empty list when it's None but never appends the provided tag, dropping the first
tag; change the logic in add_tag (referencing add_tag, self.tags, and tag) so
that when self.tags is None you create a list containing the tag (or set
self.tags = [] and then append tag), and otherwise only append tag if it's not
already in self.tags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aac1655-58c5-4d31-a127-9c726627ab1a
📒 Files selected for processing (5)
JustAnotherExpenseManager/models/__init__.pyJustAnotherExpenseManager/routes/transactions.pytests/conftest.pytests/test_app.pytests/test_integration.py
Comment on lines
+44
to
+46
| client.get('/api/transactions?page=1') | ||
| _, context = templates[0] | ||
| assert context['transactions'][0]['description'] == 'Lifecycle Expense' |
There was a problem hiding this comment.
Assert against the retrieval render, not the initial POST render.
Line 45 uses templates[0], which checks the first render. This test should validate the GET at Line 44 via templates[-1].
Suggested fix
client.get('/api/transactions?page=1')
- _, context = templates[0]
+ _, context = templates[-1]
assert context['transactions'][0]['description'] == 'Lifecycle Expense'📝 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
| client.get('/api/transactions?page=1') | |
| _, context = templates[0] | |
| assert context['transactions'][0]['description'] == 'Lifecycle Expense' | |
| client.get('/api/transactions?page=1') | |
| _, context = templates[-1] | |
| assert context['transactions'][0]['description'] == 'Lifecycle Expense' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_app.py` around lines 44 - 46, The assertion is checking the first
render (templates[0]) instead of the GET response; update the test to assert
against the retrieval render by using the last template render (templates[-1])
after calling client.get('/api/transactions?page=1'), so the line that reads
something like "_, context = templates[0]" should be changed to "_, context =
templates[-1]" to validate context['transactions'][0]['description'] ==
'Lifecycle Expense'.
| assert r.status_code == 200, r.data | ||
| _, context = templates[0] | ||
| assert context['transactions'][0]['description'] == 'Updated Transaction' | ||
| #assert b'Updated Transaction' in client.get('/api/transactions?page=1').data |
There was a problem hiding this comment.
Fix flake8 E265 comment formatting.
Line 49 and Line 54 need a space after # to satisfy the current lint rules.
Suggested fix
- `#assert` b'Updated Transaction' in client.get('/api/transactions?page=1').data
+ # assert b'Updated Transaction' in client.get('/api/transactions?page=1').data
@@
- `#assert` b'Updated Transaction' not in client.get('/api/transactions?page=1').data
+ # assert b'Updated Transaction' not in client.get('/api/transactions?page=1').dataAlso applies to: 54-54
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 49-49: block comment should start with '# '
(E265)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_integration.py` at line 49, Update the inline commented assertions
in tests/test_integration.py to satisfy flake8 E265 by adding a single space
after each '#' in the commented lines (for example change "`#assert` b'Updated
Transaction'..." to "# assert b'Updated Transaction'..." and do the same for the
other commented line around the original line 54); locate the comments by
searching for the commented assertion text "assert b'Updated Transaction'" and
the other similar commented assert and insert the space after the '#' to fix
formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
JustAnotherExpenseManager/models/__init__.py (2)
75-95:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Use
default_factoryfor callable defaults in timestamp fields.Both
created_atandupdated_atusedefault=utcnowwhich triggers the same SQLAlchemy 2.0 deprecation warning as the Tag model.🔧 Proposed fix
- created_at: Mapped[datetime] = mapped_column(default=utcnow, nullable=False, init=False) - updated_at: Mapped[datetime] = mapped_column(default=utcnow, onupdate=utcnow, nullable=False, init=False) + created_at: Mapped[datetime] = mapped_column(default_factory=utcnow, nullable=False, init=False) + updated_at: Mapped[datetime] = mapped_column(default_factory=utcnow, onupdate=utcnow, nullable=False, init=False)As per pipeline failures: SADeprecationWarning on Lines 87-88 indicates callable objects in default parameters are ambiguous in ORM-mapped dataclass context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 75 - 95, The Transaction dataclass is using callable defaults incorrectly: change the timestamp fields in the Transaction model (created_at and updated_at) to use default_factory for the utcnow callable instead of default=utcnow to avoid the SQLAlchemy deprecation warning; update the mapped_column definitions for created_at and updated_at in class Transaction to use default_factory=utcnow (and keep onupdate=utcnow for updated_at) so the callable is evaluated per-instance.
26-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Use
default_factoryfor callable defaults in ORM-mapped dataclass fields.SQLAlchemy 2.0 with typed
Mappedcolumns (dataclass-style) requiresdefault_factory=utcnowinstead ofdefault=utcnowfor callable defaults. The current code triggers deprecation warnings and will raise errors in future SQLAlchemy versions.🔧 Proposed fix
- created_at: Mapped[datetime] = mapped_column(default=utcnow, nullable=False, init=False) + created_at: Mapped[datetime] = mapped_column(default_factory=utcnow, nullable=False, init=False)As per pipeline failures: SADeprecationWarning on Line 41 indicates callable object in default parameter is ambiguous in ORM-mapped dataclass context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/models/__init__.py` around lines 26 - 42, The Tag model's dataclass-style mapped field created_at uses mapped_column(default=utcnow) which triggers SADeprecationWarning; change it to mapped_column(default_factory=utcnow, nullable=False, init=False) in the Tag class (the created_at Mapped[datetime] declaration) and similarly replace any other mapped_column(..., default=<callable>) usages with default_factory=<callable> so callable defaults are provided via default_factory per SQLAlchemy 2.0 dataclass mapping rules.
♻️ Duplicate comments (2)
tests/test_integration.py (2)
49-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix flake8 E265 comment formatting.
The comment should have a space after
#to comply with PEP 8 style guidelines.📝 Proposed fix
- `#assert` b'Updated Transaction' in client.get('/api/transactions?page=1').data + # assert b'Updated Transaction' in client.get('/api/transactions?page=1').data🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_integration.py` at line 49, The inline comment before the assertion is missing the required space after the # (flake8 E265); open the test containing the commented assertion "assert b'Updated Transaction' in client.get('/api/transactions?page=1').data" and add a single space after the # so it reads "# assert ..." to conform to PEP8/flake8 formatting.
55-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix flake8 E265 comment formatting.
The comment should have a space after
#to comply with PEP 8 style guidelines.📝 Proposed fix
- `#assert` b'Updated Transaction' not in client.get('/api/transactions?page=1').data + # assert b'Updated Transaction' not in client.get('/api/transactions?page=1').data🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_integration.py` at line 55, The commented-out assertion in tests/test_integration.py ("`#assert` b'Updated Transaction' not in client.get('/api/transactions?page=1').data") violates flake8 E265; update the comment to include a space after the hash (change "`#assert` ..." to "# assert ...") so the commented assert line complies with PEP 8 formatting; locate the commented assertion by searching for the literal "b'Updated Transaction' not in client.get" or the use of client.get('/api/transactions?page=1') in the test to apply the fix.
🧹 Nitpick comments (1)
JustAnotherExpenseManager/routes/transactions.py (1)
155-173: 💤 Low valueRedundant tag normalization after DTO construction.
Line 165 already strips and filters tags when building the DTO, so line 173 performs the same operation again unnecessarily.
♻️ Proposed simplification
entry = TransactionDTO( description=request.form.get('description', '').strip(), amount_dollars=request.form.get('amount', 0, type=float), type_str=request.form.get('type', 'expense'), date=datetime.strptime(request.form.get('date', '').split('T')[0], DT_FORMAT), category=request.form.get('category', '').lower().strip(), tags=[t.strip() for t in request.form.get('tags', '').split(',') if t.strip()] ) except ValidationError as e: return jsonify({'Validation Error': str(e)}), 400 except ValueError as e: return jsonify({'DTO Value Error': str(e)}), 400 - entry.tags = [t.strip() for t in entry.tags] if entry.tags else [] - service = TransactionService(g.db)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JustAnotherExpenseManager/routes/transactions.py` around lines 155 - 173, The tag normalization after DTO construction is redundant: in add_transaction() you already build TransactionDTO with tags computed by [t.strip() for t in request.form.get('tags', '').split(',') if t.strip()], so remove the post-construction line that reassigns entry.tags (the final entry.tags = [t.strip() for t in entry.tags] if entry.tags else []). Keep validation/error handling as-is and rely on TransactionDTO.tags from the constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Line 62: The .gitignore entry "*.xml" is too broad and hides project-critical
XMLs; replace it with more specific patterns (e.g., the exact generated/report
files you intend to ignore such as coverage.xml already covered,
test-results/*.xml, build/**/generated-*.xml, or IDE-specific XMLs) instead of
the global "*.xml" entry, updating the .gitignore rule that currently contains
"*.xml" to only target those generated or tool-specific XML filenames or paths.
In `@tests/test_routes.py`:
- Line 455: Remove the leftover debug print call print("ERROR RESPONSE:",
response.data) in the test; either delete it or replace it with proper
test-friendly logging or an assertion that includes response.data (e.g., use the
test logger or include response.data in the assertion message) so no raw print
statements remain in the test code.
---
Outside diff comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 75-95: The Transaction dataclass is using callable defaults
incorrectly: change the timestamp fields in the Transaction model (created_at
and updated_at) to use default_factory for the utcnow callable instead of
default=utcnow to avoid the SQLAlchemy deprecation warning; update the
mapped_column definitions for created_at and updated_at in class Transaction to
use default_factory=utcnow (and keep onupdate=utcnow for updated_at) so the
callable is evaluated per-instance.
- Around line 26-42: The Tag model's dataclass-style mapped field created_at
uses mapped_column(default=utcnow) which triggers SADeprecationWarning; change
it to mapped_column(default_factory=utcnow, nullable=False, init=False) in the
Tag class (the created_at Mapped[datetime] declaration) and similarly replace
any other mapped_column(..., default=<callable>) usages with
default_factory=<callable> so callable defaults are provided via default_factory
per SQLAlchemy 2.0 dataclass mapping rules.
---
Duplicate comments:
In `@tests/test_integration.py`:
- Line 49: The inline comment before the assertion is missing the required space
after the # (flake8 E265); open the test containing the commented assertion
"assert b'Updated Transaction' in client.get('/api/transactions?page=1').data"
and add a single space after the # so it reads "# assert ..." to conform to
PEP8/flake8 formatting.
- Line 55: The commented-out assertion in tests/test_integration.py ("`#assert`
b'Updated Transaction' not in client.get('/api/transactions?page=1').data")
violates flake8 E265; update the comment to include a space after the hash
(change "`#assert` ..." to "# assert ...") so the commented assert line complies
with PEP 8 formatting; locate the commented assertion by searching for the
literal "b'Updated Transaction' not in client.get" or the use of
client.get('/api/transactions?page=1') in the test to apply the fix.
---
Nitpick comments:
In `@JustAnotherExpenseManager/routes/transactions.py`:
- Around line 155-173: The tag normalization after DTO construction is
redundant: in add_transaction() you already build TransactionDTO with tags
computed by [t.strip() for t in request.form.get('tags', '').split(',') if
t.strip()], so remove the post-construction line that reassigns entry.tags (the
final entry.tags = [t.strip() for t in entry.tags] if entry.tags else []). Keep
validation/error handling as-is and rely on TransactionDTO.tags from the
constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a093e9-ed28-4658-9e2a-637b4a9d72bb
📒 Files selected for processing (8)
.gitignore.playwright/cli.config.jsonJustAnotherExpenseManager/models/__init__.pyJustAnotherExpenseManager/models/dtos.pyJustAnotherExpenseManager/routes/transactions.pyJustAnotherExpenseManager/utils/test_data.pytests/test_integration.pytests/test_routes.py
| cover/ | ||
| test-results/ | ||
| playwright-report/ | ||
| *.xml |
There was a problem hiding this comment.
The *.xml pattern is overly broad and may hide important project files.
This pattern will ignore all XML files including potential build configs (Maven pom.xml, Gradle configs), Android manifests, and other project-critical XML files. Since line 54 already covers coverage.xml, consider using more specific patterns instead.
🔧 Proposed fix
-*.xml
+junit-*.xml
+test-*.xml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore at line 62, The .gitignore entry "*.xml" is too broad and hides
project-critical XMLs; replace it with more specific patterns (e.g., the exact
generated/report files you intend to ignore such as coverage.xml already
covered, test-results/*.xml, build/**/generated-*.xml, or IDE-specific XMLs)
instead of the global "*.xml" entry, updating the .gitignore rule that currently
contains "*.xml" to only target those generated or tool-specific XML filenames
or paths.
|
|
||
| def test_populate_test_data_requires_debug_mode(self, client): | ||
| response = client.post('/api/populate-test-data') | ||
| print("ERROR RESPONSE:", response.data) |
There was a problem hiding this comment.
Remove debugging print statement before merging.
Debug print statements should not be committed to the codebase. Remove this line or replace it with proper logging if error inspection is needed.
🧹 Proposed fix
- print("ERROR RESPONSE:", response.data)📝 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
| print("ERROR RESPONSE:", response.data) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_routes.py` at line 455, Remove the leftover debug print call
print("ERROR RESPONSE:", response.data) in the test; either delete it or replace
it with proper test-friendly logging or an assertion that includes response.data
(e.g., use the test logger or include response.data in the assertion message) so
no raw print statements remain in the test code.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_models.py (2)
17-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
category='test'in every DTO is misleading dead data.The DTOs are constructed with
category='test', but everyTransaction(...)is then created withtags=[]and category tags are appended manually only where the test specifically needs them. The DTO'scategoryfield is therefore never actually exercised in these tests, and tests liketest_create_transactionend up assertingsaved.category is Nonedespite the DTO claiming'test'. Consider either:
- dropping
category='test'from DTOs that don't need it, or- building the
Transactionfrom the DTO in a way that actually applies the DTO's category (which would also catch regressions in DTO → ORM mapping).Also applies to: 52-58, 79-85, 105-111, 129-135, 220-226, 256-262, 295-301, 330-336, 345-351, 382-388, 415-421, 430-436, 463-469, 480-486, 514-520, 529-535, 544-550, 576-582, 593-599, 625-631, 639-645, 653-659, 681-687, 695-701, 709-715, 743-749
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 17 - 23, The DTOs in tests use category='test' but the code constructing Transaction instances ignores DTO.category (creates Transaction(..., tags=[])) so the DTO category is dead; update tests to either remove category='test' from TransactionDTO where it's not asserted, or modify the construction path to build Transaction from the DTO (e.g., use the DTO-to-ORM mapping function or call Transaction.from_dto/create_transaction_from_dto) so that Transaction.category is set from TransactionDTO.category; specifically review uses around TransactionDTO, Transaction, and test_create_transaction to ensure the DTO's category is actually applied or removed where not needed.
124-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix cent rounding wording vs DTO behavior
TransactionDTOderivesamount_centsusingint(value * 100)(both in theamount_dollarssetter andtrigger_computation), which truncates toward zero—so123.456789becomes12345cents, not12346. Either update the test comment to say “truncated to whole cents”, or change the DTO to use nearest-cent rounding (round(...)) and update the assertion to12346.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 124 - 125, The test and DTO disagree on cent handling: TransactionDTO currently computes amount_cents via int(value * 100) (in the amount_dollars setter and trigger_computation) which truncates; update the DTO to compute cents using nearest-cent rounding (use round(value * 100) consistently in amount_dollars setter and trigger_computation) and then update the test comment to "Should store to 2 decimal places (rounded to nearest cent)" and change the assertion to assert saved.amount_cents == 12346 so the behavior and test align.
🧹 Nitpick comments (3)
tests/test_models.py (3)
562-565: ⚡ Quick winPrefer
datetimeobjects over string literals for date-range filtering.
Transaction.dateis stored asdatetime, but the filter uses raw strings ('2026-02-01','2026-02-28'). This relies on the backend driver's implicit string→datetime coercion (works on SQLite, less portable on others) and silently excludes anything on2026-02-28after midnight. Using explicitdatetimebounds makes the intent and the inclusive/exclusive behavior unambiguous.♻️ Suggested change
- results = db.query(Transaction).filter( - Transaction.date >= '2026-02-01', - Transaction.date <= '2026-02-28' - ).all() + results = db.query(Transaction).filter( + Transaction.date >= datetime.strptime('2026-02-01', DT_FORMAT), + Transaction.date < datetime.strptime('2026-03-01', DT_FORMAT), + ).all()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 562 - 565, The test uses string literals for date-range filtering which relies on implicit coercion and excludes times on '2026-02-28' after midnight; update the db.query(...).filter(...) call to use actual datetime objects for bounds (e.g., datetime(2026,2,1) for the start and either datetime(2026,2,28,23,59,59,999999) to include the whole day or datetime(2026,3,1) with a "<" end bound) so Transaction.date comparisons are explicit and portable; import datetime at the top of the test file and replace the string literals in the filter with the chosen datetime bounds.
15-31: ⚡ Quick winConsider a small helper to remove the repeated DTO + Transaction boilerplate.
The same ~12-line pattern (
TransactionDTO(...)→Transaction(description=row.description, amount_cents=row.amount_cents, type=..., date=row.date, tags=[])) is repeated ~15 times. A single fixture/factory would shorten each test substantially, make the intent of each test clearer, and centralize the "how to build a Transaction from a DTO" knowledge.♻️ Sketch
def make_transaction(description, amount_dollars, type=TransactionType.EXPENSE, date='2026-02-01', category='test'): row = TransactionDTO( description=description, amount_dollars=amount_dollars, type=type, category=category, date=datetime.strptime(date, DT_FORMAT), ) return Transaction( description=row.description, amount_cents=row.amount_cents, type=row.type, date=row.date, tags=[], )Each test then becomes one line plus its assertions.
Also applies to: 77-92, 103-118, 127-143, 213-233, 249-269, 289-308, 330-358, 382-395, 412-443, 456-495, 512-557, 570-606, 623-666, 677-722, 736-756
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 15 - 31, Add a reusable factory/fixture to eliminate repeated DTO→model boilerplate: create a helper function (e.g., make_transaction) that accepts description, amount_dollars, type (default TransactionType.EXPENSE), date (default '2026-02-01') and category, constructs a TransactionDTO using DT_FORMAT, then returns the corresponding Transaction using row.description, row.amount_cents, row.type, row.date and tags=[]. Replace the repeated blocks that construct TransactionDTO and Transaction (seen around test_create_transaction and the other listed ranges) with calls to make_transaction to keep tests concise and centralize conversion logic.
68-69: 💤 Low valueDead
assert transaction.tags is not Nonechecks.Every occurrence is immediately preceded by
tags=[]in theTransaction(...)call, so the assertion can never fail and adds noise. Safe to remove (justtransaction.tags.append(...)directly).Also applies to: 235-237, 271-273, 310-311, 360-363, 397-399, 477-478, 494-495, 590-591, 757-759
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 68 - 69, Remove the redundant "assert transaction.tags is not None" checks that always follow constructing a Transaction(...) with tags=[]; instead, directly append to the tags list (e.g., transaction.tags.append(...)) in each failing test case where the Transaction was initialized with tags=[]. Update all occurrences where variable "transaction" is used after Transaction(...) creation (including the repeated cases noted) by deleting the assert line and leaving the append call so the tests modify the provided list without the dead assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_models.py`:
- Around line 17-23: The DTOs in tests use category='test' but the code
constructing Transaction instances ignores DTO.category (creates
Transaction(..., tags=[])) so the DTO category is dead; update tests to either
remove category='test' from TransactionDTO where it's not asserted, or modify
the construction path to build Transaction from the DTO (e.g., use the
DTO-to-ORM mapping function or call
Transaction.from_dto/create_transaction_from_dto) so that Transaction.category
is set from TransactionDTO.category; specifically review uses around
TransactionDTO, Transaction, and test_create_transaction to ensure the DTO's
category is actually applied or removed where not needed.
- Around line 124-125: The test and DTO disagree on cent handling:
TransactionDTO currently computes amount_cents via int(value * 100) (in the
amount_dollars setter and trigger_computation) which truncates; update the DTO
to compute cents using nearest-cent rounding (use round(value * 100)
consistently in amount_dollars setter and trigger_computation) and then update
the test comment to "Should store to 2 decimal places (rounded to nearest cent)"
and change the assertion to assert saved.amount_cents == 12346 so the behavior
and test align.
---
Nitpick comments:
In `@tests/test_models.py`:
- Around line 562-565: The test uses string literals for date-range filtering
which relies on implicit coercion and excludes times on '2026-02-28' after
midnight; update the db.query(...).filter(...) call to use actual datetime
objects for bounds (e.g., datetime(2026,2,1) for the start and either
datetime(2026,2,28,23,59,59,999999) to include the whole day or
datetime(2026,3,1) with a "<" end bound) so Transaction.date comparisons are
explicit and portable; import datetime at the top of the test file and replace
the string literals in the filter with the chosen datetime bounds.
- Around line 15-31: Add a reusable factory/fixture to eliminate repeated
DTO→model boilerplate: create a helper function (e.g., make_transaction) that
accepts description, amount_dollars, type (default TransactionType.EXPENSE),
date (default '2026-02-01') and category, constructs a TransactionDTO using
DT_FORMAT, then returns the corresponding Transaction using row.description,
row.amount_cents, row.type, row.date and tags=[]. Replace the repeated blocks
that construct TransactionDTO and Transaction (seen around
test_create_transaction and the other listed ranges) with calls to
make_transaction to keep tests concise and centralize conversion logic.
- Around line 68-69: Remove the redundant "assert transaction.tags is not None"
checks that always follow constructing a Transaction(...) with tags=[]; instead,
directly append to the tags list (e.g., transaction.tags.append(...)) in each
failing test case where the Transaction was initialized with tags=[]. Update all
occurrences where variable "transaction" is used after Transaction(...) creation
(including the repeated cases noted) by deleting the assert line and leaving the
append call so the tests modify the provided list without the dead assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84340623-cc20-4d02-880a-1f2a92f3fe8c
📒 Files selected for processing (1)
tests/test_models.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests