Reccur - #33
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds recurring transactions: data model and enum, scheduled background processing (APScheduler + process_recurring_transactions), CRUD routes and settings trigger, frontend templates and JS/TS UI with Tagify, build wiring, and comprehensive unit/E2E tests. ChangesRecurring Transactions Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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: 15
🧹 Nitpick comments (9)
JustAnotherExpenseManager/models/__init__.py (1)
197-201: ⚡ Quick winAdd
default_factory=listtoRecurringTransaction.tagsfor consistency.The
Transaction.tagsrelationship includesdefault_factory=list(line 101), butRecurringTransaction.tagsdoes not. This inconsistency may cause issues when accessingrt.tagsin code that expects a list (e.g., into_dict()at lines 248, theif self.tagscheck and iteration, or inprocess_recurring_transactionsat line 568 where tags are copied). While the properties handleNonegracefully, adding the default ensures consistent behavior.🔧 Suggested fix
tags: Mapped[Optional[List['Tag']]] = relationship( secondary=recurring_transaction_tags, lazy='select', + default_factory=list, init=False )🤖 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 197 - 201, RecurringTransaction.tags relationship lacks default_factory=list causing inconsistent defaults versus Transaction.tags; update the RecurringTransaction.tags relationship (the Mapped[Optional[List['Tag']]] declaration named tags) to include default_factory=list so rt.tags is initialized to an empty list by default, ensuring code paths like to_dict() (which checks/iterates self.tags) and process_recurring_transactions (where tags are copied) see a consistent list value.JustAnotherExpenseManager/utils/services.py (1)
551-591: ⚡ Quick winConsider adding error handling to prevent batch rollback on single-item failure.
If processing one recurring transaction fails (e.g., due to a database constraint violation), the entire batch rolls back and no transactions are spawned. Consider committing after each recurring item or using savepoints to isolate failures.
🛡️ Suggested approach
for rt in active_recurring: + try: while rt.next_date <= now: ... + session.commit() + except Exception as e: + session.rollback() + # Log the error for this specific recurring transaction + import logging + logging.error(f"Failed to process recurring transaction {rt.id}: {e}") + continue -session.commit()Alternatively, use nested transactions with savepoints for finer-grained control.
🤖 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 551 - 591, The loop that spawns transactions for each rt in active_recurring can cause the entire batch to roll back on a single failure; wrap the per-recurring-item work (the body that creates new_tx, copies tags, updates rt fields) in its own isolated transaction/savepoint so one failure doesn't abort others: either open a nested transaction/savepoint via session.begin_nested() around the code that adds new_tx and updates rt and commit/rollback that nested block on error, or commit after each successful rt iteration and catch exceptions to rollback only that iteration (logging the error) while continuing the loop; key symbols to change are the for rt in active_recurring loop, the Transaction creation, tag copying, rt.last_processed_date/rt.next_date updates, and the final session.commit().JustAnotherExpenseManager/templates/transactions_list.html (1)
36-40: 💤 Low valueConsider moving inline styles to CSS class.
The recurring badge uses an inline style. For consistency with the existing
type-badgeclasses and better maintainability, consider defining a.type-recurringclass in the stylesheet.♻️ Optional refactor
In your CSS file:
.type-badge.type-recurring { background: `#0984e3`; color: white; }In the template:
{% if trans.recurring_id %} - <span class="type-badge type-recurring" style="background: `#0984e3`; color: white;"> + <span class="type-badge type-recurring"> Recurring </span> {% endif %}🤖 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 36 - 40, The recurring badge currently uses an inline style in the transactions_list.html span; extract those declarations into the stylesheet by adding a .type-recurring rule (targeting .type-badge.type-recurring) with background: `#0984e3` and color: white, then remove the style attribute from the <span class="type-badge type-recurring"> so the template uses the CSS class instead of inline styles.JustAnotherExpenseManager/templates/recurring.html (1)
55-55: ⚡ Quick winPrefer addEventListener over inline event handlers.
Inline
onsubmitattributes reduce Content Security Policy effectiveness and make the code harder to maintain. Consider moving this to the JavaScript module.♻️ Proposed refactor
In the template:
- <form id="recurring-form" onsubmit="window.submitRecurring(event)"> + <form id="recurring-form">In
recurring.ts, add to the DOMContentLoaded handler:const form = document.getElementById('recurring-form') as HTMLFormElement; if (form) { form.addEventListener('submit', submitRecurring); }🤖 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/recurring.html` at line 55, Remove the inline onsubmit attribute from the form with id "recurring-form" in recurring.html and instead register the handler in the recurring.ts DOMContentLoaded initialization: find the element by id "recurring-form" and call addEventListener('submit', submitRecurring) (ensuring submitRecurring is imported/visible in that module); keep submitRecurring's signature and preventDefault logic intact so behavior doesn't change.tests/pages/RecurringPage.ts (1)
67-67: ⚡ Quick winUse
page.once()instead ofpage.on()for the dialog handler.The current dialog handler persists across the entire test and could interfere with subsequent dialog interactions. Use
once()to ensure it fires only for this deletion:- this.page.on('dialog', dialog => dialog.accept()); + this.page.once('dialog', dialog => dialog.accept());🤖 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/pages/RecurringPage.ts` at line 67, The dialog handler currently uses this.page.on('dialog', dialog => dialog.accept()) which persists across tests; change it to this.page.once('dialog', dialog => dialog.accept()) so the handler fires only for the immediate deletion action; locate the dialog registration in RecurringPage (the this.page.on(...) call) and replace .on with .once, keeping the same handler logic.tests/test_recurring.py (1)
104-104: 💤 Low valueSimplify the assertion.
The
orcondition is redundant. Iftx.next_date > today(datetime comparison) is true, the second date-only comparison is unnecessary. Simplify to:- assert tx.next_date > today or tx.next_date.date() > today.date() + assert tx.next_date > today🤖 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_recurring.py` at line 104, The assertion uses a redundant or check; replace the compound check with a single datetime comparison by changing the assertion to assert tx.next_date > today (remove the "or tx.next_date.date() > today.date()") so it relies on the datetime comparison between tx.next_date and today in the tests/test_recurring.py assertion.JustAnotherExpenseManager/routes/recurring.py (3)
6-6: 💤 Low valueRemove unused import.
The
Tagclass is imported but never directly referenced. The_get_or_create_tag()method returns aTag, but Python doesn't require the type to be imported for that usage.🧹 Cleanup
-from JustAnotherExpenseManager.models import RecurringTransaction, Tag +from JustAnotherExpenseManager.models import RecurringTransaction🤖 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/recurring.py` at line 6, Remove the unused Tag import from the top-level import line; update the import in the module so only RecurringTransaction is imported (e.g., change "from JustAnotherExpenseManager.models import RecurringTransaction, Tag" to import just RecurringTransaction), since Tag is never directly referenced (the helper _get_or_create_tag returns a Tag but does not require the type to be imported).
16-19: 💤 Low valueConsider using SQLAlchemy 2.x select() API for consistency.
The delete handler (line 59) uses
db.session.get(), while this route uses the legacydb.session.query()API. For consistency with SQLAlchemy 2.x patterns, consider refactoring to:from sqlalchemy import select `@recurring_bp.route`('/api', methods=['GET']) def list_recurring(): txs = db.session.scalars(select(RecurringTransaction)).all() return jsonify([tx.to_dict() for tx in txs])🤖 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/recurring.py` around lines 16 - 19, Replace the legacy query call in list_recurring: instead of using db.session.query(RecurringTransaction).all(), import sqlalchemy.select and use db.session.scalars(select(RecurringTransaction)).all() so the route handler list_recurring and the model RecurringTransaction follow SQLAlchemy 2.x select() API; adjust imports accordingly.
37-46: 💤 Low valueOptimize service instantiation.
TransactionServiceis instantiated separately on lines 38 and 43. When both category and tags are present, this creates two instances unnecessarily.♻️ Refactor to single service instance
+ svc = None if 'category' in data and data['category']: - svc = TransactionService(db.session) + if not svc: + svc = TransactionService(db.session) tag = svc._get_or_create_tag(f"category:{data['category']}") tx.tags.append(tag) if 'tags' in data and data['tags']: - svc = TransactionService(db.session) + if not svc: + svc = TransactionService(db.session) for t in data['tags']:Or more simply:
+ svc = TransactionService(db.session) + if 'category' in data and data['category']: - svc = TransactionService(db.session) tag = svc._get_or_create_tag(f"category:{data['category']}")🤖 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/recurring.py` around lines 37 - 46, Instantiate TransactionService once and reuse it instead of creating two instances: move the creation of svc = TransactionService(db.session) out of the two conditional blocks and use the same svc when calling svc._get_or_create_tag for both the 'category' branch (for f"category:{data['category']}") and the 'tags' loop, ensuring tx.tags.append calls remain unchanged.
🤖 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/routes/recurring.py`:
- Line 27: Replace the fragile float-to-int conversion for amount_cents
(currently int(data['amount_dollars'] * 100)) with a Decimal-based conversion to
avoid floating-point truncation: import Decimal, construct Decimal from the
original numeric string (Decimal(str(data['amount_dollars']))) or from the raw
JSON string, multiply by Decimal('100'), then convert to an integer via
to_integral_value() or int(...) so cents are correctly rounded (e.g.,
amount_cents = int((Decimal(str(data['amount_dollars'])) *
Decimal('100')).to_integral_value()) ); update the assignment to use this new
expression wherever amount_cents is computed.
- Around line 34-35: When parsing data['end_date'] into tx.end_date (using
datetime.strptime), validate it is not earlier than the transaction's start date
(tx.start_date or data['start_date'] parsed similarly); if end_date <
start_date, reject the request by raising/returning a clear validation error
(e.g., BadRequest/400) and include a descriptive message. Ensure you handle the
case where start_date may not be present or not yet parsed by
parsing/normalizing start_date before comparing, and perform the comparison
immediately after setting tx.end_date in the recurring transaction
creation/update logic.
- Around line 26-32: The code constructs a recurring record using direct
data[...] access (e.g., amount_cents=int(data['amount_dollars'] * 100],
TransactionType(data['type']), RecurringFrequency(data['frequency']),
start_date=datetime.strptime(data['start_date'], '%Y-%m-%d') ) which will raise
KeyError for missing fields; add explicit validation at the start of the route
handler that reads request JSON into data: define a required_fields list
(description, amount_dollars, type, frequency, start_date), collect any missing
= [f for f in required_fields if f not in data or data[f] in (None, '')], and if
missing raise/return a 400 Bad Request with a clear message naming the missing
fields instead of letting the generic exception handler catch it; keep the
existing conversions (amount -> amount_cents, TransactionType,
RecurringFrequency, datetime.strptime) after validation so the unique symbols
data, amount_cents=int(...), TransactionType(...), RecurringFrequency(...), and
datetime.strptime(...) remain unchanged.
- Around line 54-55: The current broad except block catches all exceptions and
returns HTTP 400; change it to distinguish client vs server errors and ensure DB
rollback: replace the single "except Exception as e:" with specific handlers
such as "except (ValueError, BadRequest) as e:" to return jsonify error with
400, "except SQLAlchemyError as e:" to call db.session.rollback() and return a
500 error, and a final "except Exception as e:" that also calls
db.session.rollback() and returns 500; reference the route's use of jsonify and
db.session.rollback()/db.session to locate where to apply these changes.
- Line 23: The assignment data = request.json can yield None and later cause
AttributeError; update the route handler (the code that reads request.json into
data) to validate that request.json (or request.get_json()) is not None before
accessing it and return a 400/Bad Request with a clear message if the body or
Content-Type is invalid; specifically check the data variable after reading
request.json, and handle the None case by returning an error response instead of
proceeding to dictionary access.
- Around line 51-53: In create_recurring, the current except ValidationError
block is unreachable because parsing/enum construction raises
ValueError/KeyError; update exception handling to catch ValueError and KeyError
(log the error and return jsonify({"error": str(e)}), 400) for client input
errors, and change the generic except Exception to call db.session.rollback(),
log the exception, and return a 500 response (e.g., jsonify({"error": "Internal
Server Error"}), 500) to match other routes; reference the create_recurring
function, the db.session.rollback() call, and the current_app.logger.error
invocations when making these changes.
In `@JustAnotherExpenseManager/templates/recurring.html`:
- Line 6: The CDN stylesheet link for Tagify in recurring.html is missing
Subresource Integrity; update the <link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/@yaireo/tagify/dist/tagify.css"> tag to
include a valid integrity="sha384-..." attribute and crossorigin="anonymous" (or
appropriate) to enable SRI checks; generate the base64 SHA-384 hash for the
exact file version (using https://www.srihash.org/ or openssl dgst -sha384
-binary tagify.js | openssl base64 -A) and paste it into the integrity attribute
so the browser rejects tampered resources.
- Around line 82-84: The tags input isn't reliably announced by assistive tech
after Tagify transforms it: add an explicit label id and reference it from the
input so screen readers keep the association — give the existing label element a
unique id (e.g., id="tags-label") and add aria-labelledby="tags-label" to the
input with id="tags" (and also add aria-placeholder or aria-describedby if you
have a helper text). Ensure you keep label for="tags" and the input id="tags"
intact so both native and ARIA associations are present.
In `@JustAnotherExpenseManager/utils/scheduler.py`:
- Around line 6-14: init_scheduler currently always initializes and starts the
APScheduler and registers the cron job; change it to skip initialization and
starting when the Flask app is in testing mode by checking
app.config.get('TESTING') (or a dedicated flag like
app.config.get('SCHEDULER_ENABLED') defaulting to True). Concretely, in
init_scheduler(app) return early (do not call scheduler.init_app,
scheduler.start, or register the `@scheduler.task`) when app.config['TESTING'] is
True; keep the existing scheduler, run_process_recurring_transactions, and
process_recurring_transactions names so the change is minimal and localized.
In `@JustAnotherExpenseManager/utils/services.py`:
- Around line 546-549: Replace the non-idiomatic equality check in the query
building for active_recurring: instead of comparing
RecurringTransaction.is_active == True, use the boolean attribute directly
(e.g., RecurringTransaction.is_active) inside the filter passed to
session.query(...) so the filter becomes RecurringTransaction.is_active and
RecurringTransaction.next_date <= now; update the code around the
active_recurring assignment to remove the "== True" comparison.
- Around line 584-589: The YEARLY branch should compute the target year and
clamp the day to the last day of that month rather than unconditionally falling
back to day=28; capture the original day (orig_day = rt.next_date.day), set
target_year = rt.next_date.year + 1, get last_day =
calendar.monthrange(target_year, rt.next_date.month)[1], then set rt.next_date =
rt.next_date.replace(year=target_year, day=min(orig_day, last_day)); update the
code around RecurringFrequency.YEARLY and rt.next_date to import/use
calendar.monthrange so Feb 29 advances correctly to Feb 28 on non-leap years
while preserving a proper day cap.
- Around line 580-583: The MONTHLY branch currently does rt.next_date =
rt.next_date + timedelta(days=days_in_month), which miscalculates month
advances; replace that logic in the elif rt.frequency ==
RecurringFrequency.MONTHLY block so you advance by one calendar month instead of
adding days — either import and use dateutil.relativedelta and do rt.next_date =
rt.next_date + relativedelta(months=1) (add python-dateutil to deps), or
implement a small helper that increments rt.next_date by one month while
clamping the day to the end of the target month (handle year rollovers and
months with fewer days) and assign the result back to rt.next_date.
In `@static_src/js/recurring.ts`:
- Around line 62-77: The rendering in loadRecurring builds raw HTML with
listDiv.innerHTML and interpolates user-controlled fields like tx.description,
causing an XSS risk; replace the string-concatenation/template-literal approach
with DOM APIs: create table/thead/tbody/tr/td elements via
document.createElement, set user data using textContent (for tx.description,
tx.frequency, tx.next_date, amount text, and tx.type) to ensure escaping, and
attach the delete action using deleteBtn.addEventListener('click', () =>
deleteRecurring(tx.id)) instead of inline onclick; update loadRecurring to clear
listDiv (e.g., listDiv.textContent = '') and append the constructed DOM tree.
- Line 107: The code currently assigns amount_dollars using
parseFloat((form.querySelector('`#amount`') as HTMLInputElement).value) without
validating the result; if parseFloat returns NaN the backend will receive an
invalid value. Fix by grabbing the amount input element (use the same
form.querySelector('`#amount`') cast), check its validity via HTML5 validity
(amountInput.validity.valid) or validate with isFinite/Number.isNaN on the
parsed value, show a user-facing error and return early if invalid, and only set
amount_dollars when the parsed number is a valid finite number.
In `@static_src/js/types.ts`:
- Around line 7-18: The RecurringTransaction interface currently declares
category as required; change it to optional to match the backend contract by
updating the RecurringTransaction definition so category?: string instead of
category: string; ensure any code referencing RecurringTransaction (e.g., form
submission logic that constructs RecurringTransaction objects) still handles
undefined or empty-string category values safely.
---
Nitpick comments:
In `@JustAnotherExpenseManager/models/__init__.py`:
- Around line 197-201: RecurringTransaction.tags relationship lacks
default_factory=list causing inconsistent defaults versus Transaction.tags;
update the RecurringTransaction.tags relationship (the
Mapped[Optional[List['Tag']]] declaration named tags) to include
default_factory=list so rt.tags is initialized to an empty list by default,
ensuring code paths like to_dict() (which checks/iterates self.tags) and
process_recurring_transactions (where tags are copied) see a consistent list
value.
In `@JustAnotherExpenseManager/routes/recurring.py`:
- Line 6: Remove the unused Tag import from the top-level import line; update
the import in the module so only RecurringTransaction is imported (e.g., change
"from JustAnotherExpenseManager.models import RecurringTransaction, Tag" to
import just RecurringTransaction), since Tag is never directly referenced (the
helper _get_or_create_tag returns a Tag but does not require the type to be
imported).
- Around line 16-19: Replace the legacy query call in list_recurring: instead of
using db.session.query(RecurringTransaction).all(), import sqlalchemy.select and
use db.session.scalars(select(RecurringTransaction)).all() so the route handler
list_recurring and the model RecurringTransaction follow SQLAlchemy 2.x select()
API; adjust imports accordingly.
- Around line 37-46: Instantiate TransactionService once and reuse it instead of
creating two instances: move the creation of svc =
TransactionService(db.session) out of the two conditional blocks and use the
same svc when calling svc._get_or_create_tag for both the 'category' branch (for
f"category:{data['category']}") and the 'tags' loop, ensuring tx.tags.append
calls remain unchanged.
In `@JustAnotherExpenseManager/templates/recurring.html`:
- Line 55: Remove the inline onsubmit attribute from the form with id
"recurring-form" in recurring.html and instead register the handler in the
recurring.ts DOMContentLoaded initialization: find the element by id
"recurring-form" and call addEventListener('submit', submitRecurring) (ensuring
submitRecurring is imported/visible in that module); keep submitRecurring's
signature and preventDefault logic intact so behavior doesn't change.
In `@JustAnotherExpenseManager/templates/transactions_list.html`:
- Around line 36-40: The recurring badge currently uses an inline style in the
transactions_list.html span; extract those declarations into the stylesheet by
adding a .type-recurring rule (targeting .type-badge.type-recurring) with
background: `#0984e3` and color: white, then remove the style attribute from the
<span class="type-badge type-recurring"> so the template uses the CSS class
instead of inline styles.
In `@JustAnotherExpenseManager/utils/services.py`:
- Around line 551-591: The loop that spawns transactions for each rt in
active_recurring can cause the entire batch to roll back on a single failure;
wrap the per-recurring-item work (the body that creates new_tx, copies tags,
updates rt fields) in its own isolated transaction/savepoint so one failure
doesn't abort others: either open a nested transaction/savepoint via
session.begin_nested() around the code that adds new_tx and updates rt and
commit/rollback that nested block on error, or commit after each successful rt
iteration and catch exceptions to rollback only that iteration (logging the
error) while continuing the loop; key symbols to change are the for rt in
active_recurring loop, the Transaction creation, tag copying,
rt.last_processed_date/rt.next_date updates, and the final session.commit().
In `@tests/pages/RecurringPage.ts`:
- Line 67: The dialog handler currently uses this.page.on('dialog', dialog =>
dialog.accept()) which persists across tests; change it to
this.page.once('dialog', dialog => dialog.accept()) so the handler fires only
for the immediate deletion action; locate the dialog registration in
RecurringPage (the this.page.on(...) call) and replace .on with .once, keeping
the same handler logic.
In `@tests/test_recurring.py`:
- Line 104: The assertion uses a redundant or check; replace the compound check
with a single datetime comparison by changing the assertion to assert
tx.next_date > today (remove the "or tx.next_date.date() > today.date()") so it
relies on the datetime comparison between tx.next_date and today in the
tests/test_recurring.py assertion.
🪄 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: 77e8b5fe-bb24-48a3-8d5f-5f43a2ddfecc
📒 Files selected for processing (19)
JustAnotherExpenseManager/__init__.pyJustAnotherExpenseManager/models/__init__.pyJustAnotherExpenseManager/models/dtos.pyJustAnotherExpenseManager/routes/recurring.pyJustAnotherExpenseManager/static/js/recurring.jsJustAnotherExpenseManager/templates/base.htmlJustAnotherExpenseManager/templates/recurring.htmlJustAnotherExpenseManager/templates/transactions_list.htmlJustAnotherExpenseManager/utils/scheduler.pyJustAnotherExpenseManager/utils/services.pypyproject.tomlstatic_src/js/recurring.tsstatic_src/js/types.tstests/12-recurring-transactions.spec.tstests/conftest.pytests/fixtures.tstests/pages/RecurringPage.tstests/test_recurring.pyvite.config.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
JustAnotherExpenseManager/routes/recurring.py (1)
24-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix set literal bug in error response.
Line 25 uses a set literal
{'error', 'Request body must be valid JSON'}instead of a dictionary. This will causejsonify()to fail or return incorrect JSON.🐛 Proposed fix
if not data: - return jsonify({'error', 'Request body must be valid JSON'}), 400 + return jsonify({'error': 'Request body must be valid JSON'}), 400🤖 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/recurring.py` around lines 24 - 25, The error response is currently built as a set literal causing jsonify to fail; update the return to use a dictionary mapping the "error" key to the message (i.e., replace the set {'error', 'Request body must be valid JSON'} with {'error': 'Request body must be valid JSON'}) where the route checks "if not data" and returns via jsonify so the response is valid JSON.
🧹 Nitpick comments (3)
static_src/js/settings.ts (2)
530-532: 💤 Low valueRemove duplicate window export.
window.exportTransactionsis assigned twice (lines 520 and 531). The duplicate on line 531 can be removed.♻️ Proposed cleanup
window.populateTestData = populateTestData; -window.exportTransactions = exportTransactions; window.runRecurringTransactions = runRecurringTransactions;🤖 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/settings.ts` around lines 530 - 532, There is a duplicate export assignment for window.exportTransactions; remove the redundant statement so each global is only set once—leave window.populateTestData and window.runRecurringTransactions as-is and delete the second window.exportTransactions assignment to avoid the duplicate global binding.
443-468: ⚖️ Poor tradeoffConsider using textContent for error messages (defense in depth).
Lines 452, 459, and 464 interpolate API response messages into
innerHTML. While the backend currently returns static strings, usingtextContentwould provide defense-in-depth protection against potential XSS if backend responses change in the future.However, this pattern is consistent with other error handling in the file (e.g.,
populateTestData,exportTransactions), so this is an optional improvement for the entire module rather than a specific issue with this function.🛡️ Optional defensive improvement
if (result.success) { if (resultDiv) { const p = document.createElement('p'); p.style.color = '`#00b894`'; p.style.fontWeight = '600'; p.textContent = `✓ ${result.message}`; resultDiv.innerHTML = ''; resultDiv.appendChild(p); setTimeout(() => { resultDiv.innerHTML = ''; }, 3000); } }🤖 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/settings.ts` around lines 443 - 468, In runRecurringTransactions replace uses of resultDiv.innerHTML that interpolate API strings with DOM-safe construction: create a <p> element, set its style properties (color, fontWeight), set p.textContent to the success message (`✓ ${result.message}`) or error text (`❌ ${(result as ApiError).error}` / `❌ Error: ${(error as Error).message}`), clear resultDiv (e.g., resultDiv.textContent = '') and append the p element, and keep the existing timeout logic; this prevents injecting untrusted HTML while preserving the current behavior and references to resultDiv, ApiResult and ApiError.tests/test_recurring.py (1)
181-206: Verify monthly end-of-month expectation: test aligns with currentrelativedelta(months=1)logic
process_recurring_transactions()advancesRecurringFrequency.MONTHLYby updatingrt.next_date = rt.next_date + relativedelta(months=1), so an end-of-month date is clamped once and that clamped day carries forward (e.g., 2023-01-31 → 2023-02-28 → 2023-03-28), matching the test expectation on line 205. If the intended UX is “last day of each month” (expecting 2023-03-31), adjust the monthly advancement to preserve the original target day/month-end each step and update the test accordingly.🤖 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_recurring.py` around lines 181 - 206, The test assumes "clamped carry-forward" behavior but your code currently uses relativedelta(months=1) in process_recurring_transactions to advance rt.next_date; decide which behavior you want and fix accordingly: either update the test to expect the clamped sequence (2023-01-31 → 2023-02-28 → 2023-03-28) or change process_recurring_transactions to implement “last day of each month” semantics by computing an original_target_day = rt.start_date.day (or store it on RecurringTransaction), then when advancing use next_month = rt.next_date + relativedelta(months=1) and set rt.next_date = date(next_month.year, next_month.month, min(original_target_day, last_day_of_month(next_month))) (use calendar.monthrange to get last_day_of_month); update references to rt.next_date, process_recurring_transactions, and RecurringTransaction.start_date accordingly.
🤖 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/routes/settings.py`:
- Around line 108-118: The /api/run-recurring endpoint (function run_recurring)
currently calls process_recurring_transactions() with no authorization; restrict
it by checking the existing _test_routes_enabled() guard or an
authentication/admin check before invoking process_recurring_transactions(),
returning a 403 if not allowed; also consider adding rate limiting/CSRF
protection hooks where similar endpoints do (see patterns used in
/api/populate-test-data and /api/transactions/clear-all) so only authorized or
test-mode requests can trigger DB updates.
In `@JustAnotherExpenseManager/static/js/settings.js`:
- Line 2227: Remove the redundant global binding by deleting the duplicate
statement that assigns window.exportTransactions = exportTransactions; and keep
only the intended new function assignment that defines/export the updated
exportTransactions implementation; locate both occurrences by searching for the
symbol exportTransactions and the duplicate window.exportTransactions assignment
and remove the earlier/duplicated one so the new function binding is the sole
export.
---
Duplicate comments:
In `@JustAnotherExpenseManager/routes/recurring.py`:
- Around line 24-25: The error response is currently built as a set literal
causing jsonify to fail; update the return to use a dictionary mapping the
"error" key to the message (i.e., replace the set {'error', 'Request body must
be valid JSON'} with {'error': 'Request body must be valid JSON'}) where the
route checks "if not data" and returns via jsonify so the response is valid
JSON.
---
Nitpick comments:
In `@static_src/js/settings.ts`:
- Around line 530-532: There is a duplicate export assignment for
window.exportTransactions; remove the redundant statement so each global is only
set once—leave window.populateTestData and window.runRecurringTransactions as-is
and delete the second window.exportTransactions assignment to avoid the
duplicate global binding.
- Around line 443-468: In runRecurringTransactions replace uses of
resultDiv.innerHTML that interpolate API strings with DOM-safe construction:
create a <p> element, set its style properties (color, fontWeight), set
p.textContent to the success message (`✓ ${result.message}`) or error text (`❌
${(result as ApiError).error}` / `❌ Error: ${(error as Error).message}`), clear
resultDiv (e.g., resultDiv.textContent = '') and append the p element, and keep
the existing timeout logic; this prevents injecting untrusted HTML while
preserving the current behavior and references to resultDiv, ApiResult and
ApiError.
In `@tests/test_recurring.py`:
- Around line 181-206: The test assumes "clamped carry-forward" behavior but
your code currently uses relativedelta(months=1) in
process_recurring_transactions to advance rt.next_date; decide which behavior
you want and fix accordingly: either update the test to expect the clamped
sequence (2023-01-31 → 2023-02-28 → 2023-03-28) or change
process_recurring_transactions to implement “last day of each month” semantics
by computing an original_target_day = rt.start_date.day (or store it on
RecurringTransaction), then when advancing use next_month = rt.next_date +
relativedelta(months=1) and set rt.next_date = date(next_month.year,
next_month.month, min(original_target_day, last_day_of_month(next_month))) (use
calendar.monthrange to get last_day_of_month); update references to
rt.next_date, process_recurring_transactions, and
RecurringTransaction.start_date accordingly.
🪄 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: edc340d5-1b4d-42b0-9ec8-1b73564d2530
📒 Files selected for processing (12)
JustAnotherExpenseManager/routes/recurring.pyJustAnotherExpenseManager/routes/settings.pyJustAnotherExpenseManager/static/js/recurring.jsJustAnotherExpenseManager/static/js/settings.jsJustAnotherExpenseManager/templates/recurring.htmlJustAnotherExpenseManager/templates/settings.htmlJustAnotherExpenseManager/utils/scheduler.pyJustAnotherExpenseManager/utils/services.pystatic_src/js/recurring.tsstatic_src/js/settings.tsstatic_src/js/types.tstests/test_recurring.py
🚧 Files skipped from review as they are similar to previous changes (5)
- static_src/js/types.ts
- JustAnotherExpenseManager/utils/scheduler.py
- JustAnotherExpenseManager/templates/recurring.html
- JustAnotherExpenseManager/utils/services.py
- JustAnotherExpenseManager/static/js/recurring.js
Comment on lines
+108
to
+118
| @settings_bp.route('/api/run-recurring', methods=['POST']) | ||
| def run_recurring() -> Union[Response, Tuple[Response, int]]: | ||
| """ | ||
| Manually trigger processing of recurring transactions. | ||
| """ | ||
| try: | ||
| from JustAnotherExpenseManager.utils.services import process_recurring_transactions | ||
| process_recurring_transactions() | ||
| return jsonify({'success': True, 'message': 'Successfully processed recurring transactions'}) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| return jsonify({'error': str(exc)}), 500 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's authentication middleware or decorators used elsewhere
rg -n "`@login_required`|`@auth_required`|`@require_auth`|check_auth|verify_auth" JustAnotherExpenseManager/routes/Repository: paperplate/JustAnotherExpenseManager
Length of output: 62
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings.py around run_recurring =="
sed -n '1,220p' JustAnotherExpenseManager/routes/settings.py | nl -ba | sed -n '70,160p'
echo
echo "== Find _test_routes_enabled usage in settings.py =="
rg -n "_test_routes_enabled" JustAnotherExpenseManager/routes/settings.py
echo
echo "== Locate /api/run-recurring references =="
rg -n "/api/run-recurring" -S JustAnotherExpenseManager
echo
echo "== Locate process_recurring_transactions definition =="
rg -n "def process_recurring_transactions" -S JustAnotherExpenseManager
echo
echo "== Show process_recurring_transactions implementation =="
python3 - <<'PY'
import os, re, subprocess, textwrap, json, sys
root="JustAnotherExpenseManager"
# Use rg to get file path and line for the def, then print a small window using sed
import subprocess
res=subprocess.check_output(["bash","-lc",f"rg -n \"def process_recurring_transactions\" -S {root}"],stderr=subprocess.STDOUT,text=True)
print(res.strip().splitlines()[0] if res.strip() else "No match")
PYRepository: paperplate/JustAnotherExpenseManager
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings.py (relevant region) =="
# Print a focused window around the provided line range
sed -n '1,220p' JustAnotherExpenseManager/routes/settings.py | cat -n | sed -n '70,160p'
echo
echo "== _test_routes_enabled usage in settings.py =="
rg -n "_test_routes_enabled" JustAnotherExpenseManager/routes/settings.py
echo
echo "== Find /api/run-recurring =="
rg -n "/api/run-recurring" -S JustAnotherExpenseManager
echo
echo "== Find process_recurring_transactions definition =="
rg -n "process_recurring_transactions" -S JustAnotherExpenseManager/utils JustAnotherExpenseManager
echo
echo "== Show process_recurring_transactions implementation (small window) =="
# Get the first matching file+line for the function definition and print nearby lines
python3 - <<'PY'
import subprocess, re
root="JustAnotherExpenseManager"
res=subprocess.check_output(["bash","-lc",f"rg -n \"def process_recurring_transactions\" -S {root}"],text=True)
first=res.strip().splitlines()[0]
m=re.match(r"^(.*?):(\\d+):", first)
if not m:
print(first); raise SystemExit
path=m.group(1); line=int(m.group(2))
start=max(1,line-40); end=line+120
print(f"Matched: {path}:{line}")
print("---- context ----")
subprocess.run(["bash","-lc",f"sed -n '{start},{end}p' {path} | cat -n | sed -n '1,180p'"],check=False)
PY
echo
echo "== Search for settings_bp registration / protection =="
rg -n "settings_bp" JustAnotherExpenseManager | head -n 50
echo
echo "== Search for auth/permission patterns outside routes =="
rg -n "before_request|current_user|g\\.|login_required|require_auth|permission|is_admin|admin" JustAnotherExpenseManager | head -n 80Repository: paperplate/JustAnotherExpenseManager
Length of output: 13976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings.py top (including _test_routes_enabled) =="
sed -n '1,120p' JustAnotherExpenseManager/routes/settings.py | cat -n
echo
echo "== settings.py around run-recurring =="
sed -n '100,140p' JustAnotherExpenseManager/routes/settings.py | cat -n
echo
echo "== services.py: process_recurring_transactions implementation (around match line) =="
# Print a window around the function definition line using sed, based on rg line number.
line=$(rg -n "def process_recurring_transactions\(" JustAnotherExpenseManager/utils/services.py | head -n 1 | sed -E 's/^[^:]+:([0-9]+):.*/\1/')
start=$((line-40)); end=$((line+220))
sed -n "${start},${end}p" JustAnotherExpenseManager/utils/services.py | cat -n
echo
echo "== scheduler.py (recurring job gating) =="
sed -n '1,120p' JustAnotherExpenseManager/utils/scheduler.py | cat -n
echo
echo "== __init__.py (app factory / before_request hooks) =="
sed -n '1,220p' JustAnotherExpenseManager/__init__.py | cat -n
echo
echo "== Search repo for auth/permission hooks outside routes =="
rg -n "before_request|current_user|g\.user|session\[|login_required|require_auth|is_admin|admin" JustAnotherExpenseManager | head -n 120Repository: paperplate/JustAnotherExpenseManager
Length of output: 6198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== process_recurring_transactions context =="
python3 - <<'PY'
import subprocess, re
root="JustAnotherExpenseManager/utils/services.py"
# find the first line number of def process_recurring_transactions(
res=subprocess.check_output(["bash","-lc",f"rg -n \"def process_recurring_transactions\\(\" {root}"],text=True).strip().splitlines()
if not res:
raise SystemExit("No match for process_recurring_transactions in services.py")
m=re.match(r"^(.*?):(\d+):", res[0])
path, line = m.group(1), int(m.group(2))
start=max(1, line-40); end=line+220
print(f"Matched: {path}:{line}")
print("---- context ----")
subprocess.run(["bash","-lc",f"sed -n '{start},{end}p' {path} | cat -n"],check=False)
PY
echo
echo "== scheduler.py =="
sed -n '1,120p' JustAnotherExpenseManager/utils/scheduler.py | cat -n
echo
echo "== __init__.py =="
sed -n '1,240p' JustAnotherExpenseManager/__init__.py | cat -n
echo
echo "== repo-wide search for before_request/auth/permission patterns =="
rg -n "before_request|`@before_request`|login_required|auth_required|require_auth|current_user|g\\.user|session\\[|is_admin|admin_required|permission|role" JustAnotherExpenseManager | head -n 200Repository: paperplate/JustAnotherExpenseManager
Length of output: 374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Exact rg match for process_recurring_transactions =="
rg -n "def[[:space:]]+process_recurring_transactions[[:space:]]*\\(" JustAnotherExpenseManager/utils/services.py
echo
echo "== services.py context around that line (first match) =="
line=$(rg -n "def[[:space:]]+process_recurring_transactions[[:space:]]*\\(" JustAnotherExpenseManager/utils/services.py | head -n 1 | sed -E 's/^[^:]+:([0-9]+):.*/\1/')
start=$((line-60))
end=$((line+260))
sed -n "${start},${end}p" JustAnotherExpenseManager/utils/services.py | cat -n
echo
echo "== scheduler.py =="
sed -n '1,120p' JustAnotherExpenseManager/utils/scheduler.py | cat -n
echo
echo "== __init__.py app factory =="
sed -n '1,240p' JustAnotherExpenseManager/__init__.py | cat -n
echo
echo "== repo-wide search for request auth gating =="
rg -n "before_request|`@before_request`|login_required|require_auth|auth_required|current_user|session\\[|is_admin|admin_required|permission|role" JustAnotherExpenseManager | head -n 200Repository: paperplate/JustAnotherExpenseManager
Length of output: 594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== services.py context around process_recurring_transactions (line 536) =="
sed -n '476,650p' JustAnotherExpenseManager/utils/services.py | cat -n
echo
echo "== scheduler.py =="
sed -n '1,160p' JustAnotherExpenseManager/utils/scheduler.py | cat -n
echo
echo "== __init__.py app factory =="
sed -n '1,260p' JustAnotherExpenseManager/__init__.py | cat -n
echo
echo "== settings_bp protection / request hooks search =="
rg -n "before_request|teardown_request|`@settings_bp`|register_blueprint\\(settings_bp\\)|login_required|require_auth|auth_required|current_user|session\\[|g\\.user|is_admin|admin_required|permission|role" JustAnotherExpenseManager | head -n 200Repository: paperplate/JustAnotherExpenseManager
Length of output: 16851
Add authorization (or test-mode gating) to /api/run-recurring to prevent unauthenticated DB mutations.
JustAnotherExpenseManager/routes/settings.py(POST /api/run-recurring, lines ~108-118) callsprocess_recurring_transactions()directly and is not guarded by_test_routes_enabled()or any auth check (unlike/api/populate-test-dataand/api/transactions/clear-all).JustAnotherExpenseManager/utils/services.py::process_recurring_transactions()creates newTransactionrows for dueRecurringTransactions and updatesnext_date/last_processed_datein the database, so anyone can force recurring transaction processing on demand.
Restrict this endpoint to authenticated/admin users (or gate it to debug/testing), and consider rate limiting (and CSRF protection if it uses cookie-based sessions).
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 117-117: Do not catch blind exception: Exception
(BLE001)
🤖 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/settings.py` around lines 108 - 118, The
/api/run-recurring endpoint (function run_recurring) currently calls
process_recurring_transactions() with no authorization; restrict it by checking
the existing _test_routes_enabled() guard or an authentication/admin check
before invoking process_recurring_transactions(), returning a 403 if not
allowed; also consider adding rate limiting/CSRF protection hooks where similar
endpoints do (see patterns used in /api/populate-test-data and
/api/transactions/clear-all) so only authorized or test-mode requests can
trigger DB updates.
| window.saveEditTag = saveEditTag; | ||
| window.deleteTag = deleteTag; | ||
| window.populateTestData = populateTestData; | ||
| window.exportTransactions = exportTransactions; |
There was a problem hiding this comment.
Remove duplicate window assignment.
Line 2227 duplicates the assignment from line 2217. Only the new function assignment on line 2228 is needed here.
🧹 Proposed fix
-window.exportTransactions = exportTransactions;
window.runRecurringTransactions = runRecurringTransactions;🤖 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/settings.js` at line 2227, Remove the
redundant global binding by deleting the duplicate statement that assigns
window.exportTransactions = exportTransactions; and keep only the intended new
function assignment that defines/export the updated exportTransactions
implementation; locate both occurrences by searching for the symbol
exportTransactions and the duplicate window.exportTransactions assignment and
remove the earlier/duplicated one so the new function binding is the sole
export.
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.
Implement #28
Summary by CodeRabbit
New Features
Tests