Skip to content

Conversation

@luckyPipewrench
Copy link
Collaborator

@luckyPipewrench luckyPipewrench commented Nov 23, 2025

◦ Pending detection: honor provider pending: true; infer when posted blank/0 and transacted_at present. Avoid epoch by treating zero posted as missing.

◦ FX metadata: record extra.simplefin.fx_from and fx_date (prefers transacted_at, fallback posted) when tx currency != account currency.

◦ UI: subtle, uniform “Pending” badge when either extra.simplefin.pending or extra.plaid.pending is true.

◦ Debugging: optional SIMPLEFIN_INCLUDE_PENDING and SIMPLEFIN_DEBUG_RAW, both default‑off.

Summary by CodeRabbit

  • New Features
    • Added a pending-status visual indicator (clock icon and badge) for pending transactions.
    • Improved pending detection and FX metadata capture for multi-currency transactions.
  • Bug Fixes
    • Treats zero/“0” posted dates as missing to avoid incorrect epoch dates for pending items.
  • Tests
    • Added unit tests covering pending detection and FX handling.
  • Documentation
    • Added provider docs describing pending rules and optional debug flags.

✏️ Tip: You can customize this high-level summary in your review settings.

…vior via ENV.

- Enhance debug logging for SimpleFin API requests and raw payloads.
- Refine pending flag handling in `SimplefinEntry::Processor` based on provider data and inferred conditions.
- Improve FX metadata processing for transactions with currency mismatches.
- Add new tests for pending detection, FX metadata, and edge cases involving `posted` values.
- Add pending indicator UI to transaction view.
@coderabbitai
Copy link

coderabbitai bot commented Nov 23, 2025

Walkthrough

Adds pending-transaction detection and FX metadata extraction to Simplefin entry processing, makes importer pending inclusion driven by ENV or explicit flag and adds raw-payload debug logging, shows a Pending badge in transaction views, and includes unit tests and documentation updates.

Changes

Cohort / File(s) Summary
Processor: pending & FX metadata
app/models/simplefin_entry/processor.rb
Adds pending detection to extra_metadata (provider flag OR missing/zero posted with transacted_at), treats 0/"0" posted as missing, and records FX metadata (fx_from, fx_date) when tx currency differs from account currency.
Importer: pending flag & debug logging
app/models/simplefin_item/importer.rb
Derives effective pending from pending param or ENV["SIMPLEFIN_INCLUDE_PENDING"] (accepts 1/true/yes/on), includes pending in request logs, and conditionally logs raw payload when ENV["SIMPLEFIN_DEBUG_RAW"] is set.
UI: pending indicator
app/views/transactions/_transaction.html.erb
Renders a "Pending" badge when transaction.extra indicates pending (checks provider-specific paths), with safe extraction and error-rescue to no-op on failure.
Tests
test/models/simplefin_entry/processor_test.rb
Adds tests for pending detection (posted nil or zero, provider flag precedence) and FX metadata capture (currency mismatch uses transacted_at).
Docs & comments
AGENTS.md, CLAUDE.md, app/models/provider/simplefin.rb
Adds documentation for pending/FX behavior and in-code comments about provider variability and new ENV debug flags.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Extra attention:
    • app/models/simplefin_entry/processor.rb — ordering/precedence of pending detection and handling of posted == 0/"0".
    • app/models/simplefin_item/importer.rb — parsing of ENV["SIMPLEFIN_INCLUDE_PENDING"] truthy values and logging side-effects.
    • app/views/transactions/_transaction.html.erb — robustness of nested extra extraction and HTML rendering/rescue behavior.
    • Test coverage consistency with edge cases (timezones, nil/empty strings).

Possibly related PRs

Suggested reviewers

  • jjmata

Poem

🐰 I sniffed a pending flag in flight,
I tunneled FX dates into the light,
ENV whispers which transactions to keep,
Badges now tick where balances sleep,
Tests hop beside — all tidy and bright.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely summarizes the three main changes in this pull request: pending detection logic, FX metadata tracking, and the pending UI badge.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7a813d and 73e61d6.

📒 Files selected for processing (3)
  • AGENTS.md (1 hunks)
  • CLAUDE.md (1 hunks)
  • app/models/provider/simplefin.rb (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • app/models/provider/simplefin.rb
  • AGENTS.md
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/jobs/**/*.rb : Handle account syncing, import processing, and AI responses via background jobs (Sidekiq)

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: CSV import is managed via `Import` model with support for transaction and balance imports, custom field mapping, and transformation rules.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Use Sidekiq for background processing including account syncing, import processing, AI chat responses, and scheduled maintenance via sidekiq-cron.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Core domain model: User → Accounts → Transactions. Accounts support types: checking, savings, credit cards, investments, crypto, loans, properties. Transactions belong to Categories and may have Tags and Rules.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Use PlaidItem to manage Plaid connections and Sync to track sync operations for bank account syncing

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.

Applied to files:

  • CLAUDE.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: ci / test
🔇 Additional comments (1)
CLAUDE.md (1)

97-102: Clarify pending detection edge cases and verify i18n for badge text.

  1. "No badge if provider omits pendings" – clarify whether this means the provider doesn't return a pending field at all, or if the field is explicitly false. The current phrasing is ambiguous.

  2. Posted=0 handling – the documentation says "posted blank/0" but doesn't explain why this distinction matters. Consider: "posted blank/0 (to avoid treating epoch dates as valid transaction dates)".

  3. i18n for "Pending" badge – per project conventions, all user-facing strings must use localization. Verify that the "Pending" badge text is i18n'd (e.g., t("transactions.status.pending")) rather than hardcoded.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@luckyPipewrench luckyPipewrench marked this pull request as ready for review November 23, 2025 22:10
@jjmata
Copy link
Collaborator

jjmata commented Nov 24, 2025

I don't have pending transactions to try and test this with ... do they happen often for you, @luckyPipewrench?

@luckyPipewrench
Copy link
Collaborator Author

I have a bunch. Some providers don't send pending txs in the raw json at all though, eg capitalone.

@luckyPipewrench
Copy link
Collaborator Author

image

@jjmata
Copy link
Collaborator

jjmata commented Dec 6, 2025

Did we decide to leave pending out for now, then? Probably good to document in the Providers comments and in all the LLM-fu documents (AGENTS.md - CLAUDE.md - etc.)

…SimpleFIN and Plaid integrations. Add debug flags for troubleshooting.
@luckyPipewrench
Copy link
Collaborator Author

luckyPipewrench commented Dec 6, 2025

I'd like to have the pending feature in. It’s passive and only surfaces when the provider includes signals (pending: true or posted: 0 + transacted_at). If a provider doesn’t send pendings (e.g., Capital One), nothing changes in the UI. I’ve added brief docs to AGENTS.md and CLAUDE.md explaining detection rules, provider variability, and the optional ENV flags for debugging. If you’d prefer we gate the badge behind a config, I can add that, but I think it’s safe as-is. Can also make the badge generic by scanning all provider namespaces for a truthy pending key in transaction.extra rather than checking only SimpleFIN/Plaid. That would make new providers work without UI edits, or just list providers explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants