feat(statements): add account statement vault#1675
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:
📝 WalkthroughWalkthroughAdds a new Account Statements feature (model, migrations, controller, views, helpers, metadata detection, matching, coverage, Active Storage auth, routes, tests) and small devcontainer/docker-compose tweaks for libvips and Selenium node configuration. Wires ChangesAccount Statements Feature
Devcontainer & CI Tooling Tweaks
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as AccountStatementsController
participant Model as AccountStatement
participant Detector as MetadataDetector
participant Matcher as AccountMatcher
participant Storage as ActiveStorage
participant DB as Database
User->>Controller: POST /accounts/:id/account_statements (upload files)
Controller->>Model: prepare_upload!(file)
Model->>Storage: read content, compute checksum/content-type
Model-->>Controller: PreparedUpload
Controller->>Model: duplicate_for(family, prepared_upload)
Model->>DB: query by family & content_sha256/checksum
alt Duplicate
Controller-->>User: flash duplicate, skip create
else New
Controller->>Model: create_from_prepared_upload!
Model->>Detector: apply (extract period/hints)
Detector-->>Model: period_start_on, period_end_on, hints
Model->>Storage: attach original_file
Model->>Matcher: best_match
Matcher->>DB: fetch visible accounts, compute confidences
Matcher-->>Model: Match (account/confidence)
Model->>DB: insert AccountStatement row
Controller-->>User: redirect with success
end
sequenceDiagram
actor User
participant Controller as AccountStatementsController
participant Coverage as AccountStatement::Coverage
participant DB as Database
participant View as Browser
User->>Controller: GET /accounts/:id?tab=statements&statement_year=YYYY
Controller->>DB: fetch statements for account
Controller->>Coverage: Coverage.for_year(account, YYYY)
Coverage->>DB: query statements within year range
loop months in year
Coverage->>DB: compute balances/reconciliation per month
Coverage-->>Coverage: build Month struct with status
end
Coverage-->>Controller: coverage months + summary
Controller->>View: render statements tab with calendar and tables
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Thanks for the detailed write-up. A few things worth discussing before this moves out of draft: Scope: At 2,733 additions across 30 files, this is a large surface area for a first contribution — new model, migrations, ActiveStorage, controller, views, UI, coverage/reconciliation logic, and authorization. The related PR #1382 explored similar territory and wasn't merged. It would help to get maintainer sign-off on the Trust flag: Your contributor profile is flagged by the automated trust scanner (score 68/100 — low identity and graph scores). This doesn't reflect on the PR content, but it means maintainers will want to review this extra carefully, especially the ActiveStorage authorization paths and the file-type validation (MIME/extension/PDF magic header checks). Suggestion: Consider breaking this into a smaller initial PR with just the model + migration + upload/download authorization, get that reviewed, then layer the UI and coverage logic on top. The PR notes say "V1 foundation-first" — that philosophy could also apply to PR sizing. Generated by Claude Code Generated by Claude Code |
|
Large, well-structured PR. The migration strategy (add constraints unvalidated, validate in a separate no-DDL-transaction migration) is exactly right for zero-downtime. A few things worth discussing: MD5 as the primary deduplication key
def initialize(account, start_month: 11.months.ago.to_date.beginning_of_month, end_month: Date.current.beginning_of_month)
Active Storage authorization refactoring is clean The new Generated by Claude Code |
brin-security-scanner
Bot
added
contributor:flagged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84e594ae8e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
.devcontainer/Dockerfile (1)
14-14: 💤 Low value
libvips42is correctly named for Bookworm, but consider whether it's necessaryVerified that
libvips42is a valid package on Debian Bookworm. However, since the PR's ActiveStorage attachments are non-image files (PDF/CSV/XLSX), this dependency appears precautionary — the app already hasimagemagickfor image processing. Confirm this addition is intentional, or remove it if image variant generation isn't used elsewhere in the application.🤖 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 @.devcontainer/Dockerfile at line 14, The Dockerfile currently installs libvips42 but the PR indicates attachments are non-image types; verify whether ActiveStorage image variant generation or the image_processing gem (or any code referencing Vips/MiniMagick) is actually used—search for calls to variant, processed, ImageProcessing, MiniMagick, or Vips in the codebase and check Gemfile for image-processing gems; if not used, remove the libvips42 entry from .devcontainer/Dockerfile and rebuild images, otherwise keep it and add a brief inline comment documenting why libvips42 is required for image variants so its presence is intentional.app/helpers/settings_helper.rb (1)
5-5: ⚡ Quick winLocalize helper nav labels instead of hardcoding display text.
This adds another user-facing English string in Ruby (
name: "Statement Vault"), which is later rendered in nav/footer titles. Prefer storing a translation key (or resolving label viat(...)) for this entry.As per coding guidelines: "Always use
t()helper for user-facing strings... Updateconfig/locales/en.ymlfor new strings."🤖 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 `@app/helpers/settings_helper.rb` at line 5, Replace the hardcoded label "Statement Vault" in the nav entry (the hash with keys name, path: :account_statements_path, condition: :admin_user?) with a translation lookup using the t(...) helper (e.g., t('settings.statement_vault') or an appropriate scope you use for helper labels), and add the corresponding key and English value to config/locales/en.yml so the UI string is localized.app/controllers/account_statements_controller.rb (1)
142-148: 💤 Low value
ensure_statement_manager!return value is unused.In Rails 5+,
before_actionhalts only on explicitrender/redirect_to, not on a falsy return. Thereturn true/falseare noise; the redirect is what halts the chain. Optional cleanup.♻️ Suggested cleanup
def ensure_statement_manager! - return true if AccountStatement.statement_manager?(Current.user) + return if AccountStatement.statement_manager?(Current.user) redirect_to accounts_path, alert: t("accounts.not_authorized") - false end🤖 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 `@app/controllers/account_statements_controller.rb` around lines 142 - 148, The ensure_statement_manager! method returns true/false which are unused because before_action in Rails halts only on render/redirect; remove the "return true" and "false" return values and simplify the method so it only redirects when AccountStatement.statement_manager?(Current.user) is false (e.g., call redirect_to accounts_path, alert: t("accounts.not_authorized") and let the method end), leaving the authorization check in ensure_statement_manager! unchanged.test/controllers/accounts_controller_test.rb (1)
356-367: ⚡ Quick winDRY: extract the duplicated
uploaded_filetest helper.The same helper is now defined in three places in this PR:
test/controllers/accounts_controller_test.rb(here, returnsActionDispatch::Http::UploadedFile)test/integration/active_storage_authorization_test.rb(returnsActionDispatch::Http::UploadedFile)test/controllers/account_statements_controller_test.rb(returnsRack::Test::UploadedFile— already divergent)The implementations have already drifted. Consider hoisting a single helper into
test/test_helper.rb(or atest/support/module included intoActiveSupport::TestCase) so all three suites build uploads the same way and bug fixes apply once.🤖 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 `@test/controllers/accounts_controller_test.rb` around lines 356 - 367, The duplicated uploaded_file helper (method uploaded_file in accounts_controller_test.rb) should be extracted to a single shared helper (e.g., define uploaded_file in test/test_helper.rb or a module under test/support and include it into ActiveSupport::TestCase) so all tests use the same implementation and return type (choose one: ActionDispatch::Http::UploadedFile or Rack::Test::UploadedFile) and update the three callers (accounts_controller_test.rb, active_storage_authorization_test.rb, account_statements_controller_test.rb) to remove their local definitions and rely on the shared uploaded_file helper; ensure the shared helper creates the Tempfile, sets binmode, writes/rewinds content, and returns the chosen uploaded file type so behavior is consistent across suites.app/models/account.rb (1)
24-24: 💤 Low valueOptional:
dependent: :nullifyis redundant with the explicit callback.The custom
before_destroy :move_account_statements_to_inbox(line 5) is registered before thehas_manyis declared, so it runs first and clearsaccount_id/review_status/match_confidencein a singleUPDATE. The auto-generateddependent: :nullifycallback then issues a secondUPDATEthat matches zero rows. It's harmless but adds a wasted query per account destroy and dilutes intent (the explicit callback does strictly more than nullify).Consider dropping
dependent: :nullifyand keeping the explicit callback as the single source of truth, or vice-versa.♻️ Suggested change
- has_many :account_statements, dependent: :nullify + has_many :account_statements🤖 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 `@app/models/account.rb` at line 24, The has_many declaration includes dependent: :nullify which is redundant because the custom before_destroy callback move_account_statements_to_inbox already nulllifies account_id/review_status/match_confidence in one UPDATE; remove dependent: :nullify from the has_many :account_statements declaration so only the explicit before_destroy :move_account_statements_to_inbox callback runs (or, if you prefer the built-in behavior, delete the before_destroy callback instead) — update the has_many :account_statements line and ensure move_account_statements_to_inbox remains or is removed consistently.app/views/accounts/show/_statements.html.erb (1)
4-4: 💤 Low valueAvoid the extra
COUNT(*)query — prefer.sizeafter loading.
statementsis a relation.statements.counton line 65 always issues aCOUNT(*), andstatements.any?/statements.eachlater trigger a separateSELECT. Loading once and using.sizecollapses this to a single query.♻️ Suggested change
-<% statements = account.account_statements.with_attached_original_file.ordered %> +<% statements = account.account_statements.with_attached_original_file.ordered.to_a %>…then
statements.count/.any?/.eachall use the loaded array.Also applies to: 65-65
🤖 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 `@app/views/accounts/show/_statements.html.erb` at line 4, The view builds `statements` as an ActiveRecord relation which causes extra COUNT(*) queries when you later call `statements.count` and then iterate; call `to_a` (or `load`) when assigning — e.g. change the assignment in the `_statements.html.erb` fragment where `statements = account.account_statements.with_attached_original_file.ordered` to `statements = account.account_statements.with_attached_original_file.ordered.to_a` and update uses of `statements.count` to `statements.size` so the count/any?/each operate on the already-loaded array rather than issuing extra SQL.app/models/account_statement.rb (2)
193-201: ⚡ Quick win
unlink!performs two non-atomic writes; wrap in a transaction.
update!on line 194 commits the unlink, thenmatch_account!mutatessuggested_account/match_confidencein memory, and a secondsave!commits those. If the secondsave!validates-fails (e.g., a staleoriginal_fileconstraint or a concurrent edit), the row is left unlinked but without the recomputed suggestion — partial state is observable to readers. Group both writes in a transaction (or merge the suggestion into the initialupdate!after computing it).Proposed fix
def unlink! - update!( - account: nil, - review_status: :unmatched, - match_confidence: nil - ) - match_account! - save! + transaction do + update!( + account: nil, + review_status: :unmatched, + match_confidence: nil + ) + match_account! + save! + end end🤖 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 `@app/models/account_statement.rb` around lines 193 - 201, unlink! performs two separate DB writes (update! then save!), risking partial state if the second save! fails; wrap the operations in a single transaction so both changes commit atomically. Modify unlink! to begin an ActiveRecord transaction around the call to update! and the subsequent match_account!/save! (or compute suggested_account/match_confidence first and include them in the initial update!), referencing unlink!, match_account!, suggested_account, match_confidence, update! and save! so the unlink and recomputed suggestion persist or roll back together.
211-216: 💤 Low valueMisleading bang on
match_account!— method does not persist.By Ruby convention
!signals a method that mutates state in a "dangerous" way (raises on failure, persists, etc.).match_account!only assignssuggested_accountandmatch_confidencein memory and relies on the caller (unlink!,create_from_prepared_upload!) to callsave!. Renaming tomatch_account(orassign_account_match) avoids surprising callers who expect a write.🤖 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 `@app/models/account_statement.rb` around lines 211 - 216, The method match_account! misleadingly uses a bang but does not persist; rename match_account! to a non-bang name (e.g. match_account or assign_account_match) and update all call sites (including unlink! and create_from_prepared_upload!) to call the new method; ensure the method still assigns suggested_account and match_confidence on self and do not change persistence behavior (leave save!/persisting calls in the callers).app/helpers/account_statements_helper.rb (1)
17-32: ⚡ Quick winPrefer sure-design-system functional tokens over raw Tailwind color utilities.
The
covered,duplicate,ambiguous, andmismatchedbranches use raw Tailwind color utilities (bg-green-500/10,text-green-600,ring-green-500/20,bg-orange-500/10,text-red-600, etc.), whilenot_expectedand the fallback already use functional tokens (bg-container-inset,text-subdued,text-secondary,bg-gray-tint-5,ring-alpha-black-25). Mixing both in the same helper makes theming inconsistent and bypasses the design system. Consider mapping each status to a status-named functional token (e.g., success/warning/danger surfaces) defined insure-design-system.cssso dark-mode and theme overrides apply uniformly.As per coding guidelines: "Always prefer using functional tokens defined in sure-design-system.css (e.g., text-primary, bg-container, border-primary) rather than raw Tailwind utilities or arbitrary colors".
🤖 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 `@app/helpers/account_statements_helper.rb` around lines 17 - 32, The helper account_statement_coverage_classes mixes raw Tailwind colors with sure-design-system functional tokens; update the "covered", "duplicate", "ambiguous", and "mismatched" branches to use the design-system's functional tokens instead of raw utilities so theming/dark-mode works uniformly. Replace strings like "bg-green-500/10 text-green-600 ring-green-500/20" etc. with the corresponding functional tokens defined in sure-design-system.css (for example use the status-specific surface/text/ring tokens such as the success/warning/danger surface and text tokens used elsewhere), and ensure the fallback branches keep the existing functional tokens pattern. Verify token names against sure-design-system.css and keep the same return shape (a single space-separated class string) in account_statement_coverage_classes.
🤖 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 `@app/controllers/account_statements_controller.rb`:
- Around line 54-67: The loop currently returns early on
ActiveRecord::RecordInvalid, discarding previously collected created/duplicates;
instead initialize a validation_errors = [] before prepared_uploads, rescue
ActiveRecord::RecordInvalid => e by appending
e.record.errors.full_messages.to_sentence (or similar) to validation_errors and
continue (do not return), and after the loop pass validation_errors into the
final redirect (either by extending flash_for_upload to accept a
validation_errors argument or by merging validation_errors into the flash/alert
alongside flash_for_upload(created:, duplicates:)); reference prepared_uploads,
AccountStatement.create_from_prepared_upload!, ActiveRecord::RecordInvalid,
flash_for_upload, and redirect_after_create when making these changes.
In `@app/models/account_statement.rb`:
- Around line 109-114: The rescue for ActiveRecord::RecordNotUnique contains a
dead guard because prepared_upload is a required keyword of
create_from_prepared_upload!, so defined?(prepared_upload) is always true;
remove the defined? check and either (A) simplify by calling duplicate =
duplicate_for(family, prepared_upload) then raise DuplicateUploadError,
duplicate if duplicate (dropping the guard), or (B) if you intended to only
handle duplicates from this upload, narrow the rescue to the specific DB
constraint (or inspect exception.constraint_name) before invoking duplicate_for;
update the rescue block around create_from_prepared_upload! accordingly and keep
references to duplicate_for, DuplicateUploadError, and the
ActiveRecord::RecordNotUnique rescue.
- Around line 37-65: The custom scopes scope :linked and scope :unmatched are
shadowing the enum :review_status generated scopes (and leaving the enum
predicate linked? with different semantics), causing surprising behavior; to
fix, either disable the enum-generated scopes by changing enum :review_status, {
... }, _scopes: false and keep your custom scopes, or rename the custom scopes
(e.g., scope :with_account instead of :linked and scope :unmatched_inbox instead
of :unmatched) and update all call sites and tests accordingly so enum methods
(linked?, AccountStatement.linked) and your intentions are no longer ambiguous;
update any code using linked? or AccountStatement.linked to the new names if you
choose rename.
- Around line 116-136: prepare_upload! currently reads the entire file before
enforcing MAX_FILE_SIZE; change it to check the declared size first (e.g. if
file.respond_to?(:size) or file.respond_to?(:length) and file.size.present?) and
raise InvalidUploadError immediately when size > MAX_FILE_SIZE, and for IOs with
no declared size, stream-read the payload in chunks up to MAX_FILE_SIZE+1 to
detect oversize before buffering (stop and raise if exceeded), then rewind as
needed and proceed to compute byte_size, checksum and content_sha256 and call
detected_content_type and PreparedUpload as before; ensure references to
prepare_upload!, MAX_FILE_SIZE, detected_content_type, and PreparedUpload are
updated accordingly.
In `@app/models/account_statement/metadata_detector.rb`:
- Around line 7-22: DATE_PATTERNS and MONTH_PATTERN currently rely on \b which
fails for underscore-separated filenames; replace the \b anchors with negative
lookbehind/negative lookahead patterns that assert the char before/after is not
[a-z0-9] (e.g. (?<![a-z0-9]) and (?![a-z0-9])) so underscores are treated as
valid separators while still preventing letter/number adjacency; also make
parse_date perform the same plausibility check as parse_date_with_format by
invoking reasonable_date? on the parsed date so filename-derived dates are
validated consistently with CSV-derived dates.
In `@app/views/account_statements/show.html.erb`:
- Around line 44-64: Remove the account_id select from the metadata form to
avoid bypassing strong params: delete the form.collection_select :account_id
line inside the styled_form_with block (the block guarded by
`@can_manage_statement`) so account linking is handled only via the explicit
Link/Unlink/Reject controls; keep the controller-side statement_account_id logic
as-is (or update any tests) so no raw params are relied upon from this form and
statement_params remains the sole mass-assignment source for the other fields.
In `@test/integration/active_storage_authorization_test.rb`:
- Around line 102-118: The test "user can access linked statement blob for a
shared account" is too weak because assert_response :redirect only checks for
any 3xx; update the assertion to follow the redirect and assert the final
location is the actual disk URL like the other family-access test does: after
signing in users(:family_member) and calling get
rails_blob_path(statement.original_file), follow the redirect response (use the
same follow-redirect pattern as the "user can access attachments within their
own family" test) and assert the final URL points to the blob's service/disk URL
rather than the login page or another route so the test fails closed if
authorization regresses.
---
Nitpick comments:
In @.devcontainer/Dockerfile:
- Line 14: The Dockerfile currently installs libvips42 but the PR indicates
attachments are non-image types; verify whether ActiveStorage image variant
generation or the image_processing gem (or any code referencing Vips/MiniMagick)
is actually used—search for calls to variant, processed, ImageProcessing,
MiniMagick, or Vips in the codebase and check Gemfile for image-processing gems;
if not used, remove the libvips42 entry from .devcontainer/Dockerfile and
rebuild images, otherwise keep it and add a brief inline comment documenting why
libvips42 is required for image variants so its presence is intentional.
In `@app/controllers/account_statements_controller.rb`:
- Around line 142-148: The ensure_statement_manager! method returns true/false
which are unused because before_action in Rails halts only on render/redirect;
remove the "return true" and "false" return values and simplify the method so it
only redirects when AccountStatement.statement_manager?(Current.user) is false
(e.g., call redirect_to accounts_path, alert: t("accounts.not_authorized") and
let the method end), leaving the authorization check in
ensure_statement_manager! unchanged.
In `@app/helpers/account_statements_helper.rb`:
- Around line 17-32: The helper account_statement_coverage_classes mixes raw
Tailwind colors with sure-design-system functional tokens; update the "covered",
"duplicate", "ambiguous", and "mismatched" branches to use the design-system's
functional tokens instead of raw utilities so theming/dark-mode works uniformly.
Replace strings like "bg-green-500/10 text-green-600 ring-green-500/20" etc.
with the corresponding functional tokens defined in sure-design-system.css (for
example use the status-specific surface/text/ring tokens such as the
success/warning/danger surface and text tokens used elsewhere), and ensure the
fallback branches keep the existing functional tokens pattern. Verify token
names against sure-design-system.css and keep the same return shape (a single
space-separated class string) in account_statement_coverage_classes.
In `@app/helpers/settings_helper.rb`:
- Line 5: Replace the hardcoded label "Statement Vault" in the nav entry (the
hash with keys name, path: :account_statements_path, condition: :admin_user?)
with a translation lookup using the t(...) helper (e.g.,
t('settings.statement_vault') or an appropriate scope you use for helper
labels), and add the corresponding key and English value to
config/locales/en.yml so the UI string is localized.
In `@app/models/account_statement.rb`:
- Around line 193-201: unlink! performs two separate DB writes (update! then
save!), risking partial state if the second save! fails; wrap the operations in
a single transaction so both changes commit atomically. Modify unlink! to begin
an ActiveRecord transaction around the call to update! and the subsequent
match_account!/save! (or compute suggested_account/match_confidence first and
include them in the initial update!), referencing unlink!, match_account!,
suggested_account, match_confidence, update! and save! so the unlink and
recomputed suggestion persist or roll back together.
- Around line 211-216: The method match_account! misleadingly uses a bang but
does not persist; rename match_account! to a non-bang name (e.g. match_account
or assign_account_match) and update all call sites (including unlink! and
create_from_prepared_upload!) to call the new method; ensure the method still
assigns suggested_account and match_confidence on self and do not change
persistence behavior (leave save!/persisting calls in the callers).
In `@app/models/account.rb`:
- Line 24: The has_many declaration includes dependent: :nullify which is
redundant because the custom before_destroy callback
move_account_statements_to_inbox already nulllifies
account_id/review_status/match_confidence in one UPDATE; remove dependent:
:nullify from the has_many :account_statements declaration so only the explicit
before_destroy :move_account_statements_to_inbox callback runs (or, if you
prefer the built-in behavior, delete the before_destroy callback instead) —
update the has_many :account_statements line and ensure
move_account_statements_to_inbox remains or is removed consistently.
In `@app/views/accounts/show/_statements.html.erb`:
- Line 4: The view builds `statements` as an ActiveRecord relation which causes
extra COUNT(*) queries when you later call `statements.count` and then iterate;
call `to_a` (or `load`) when assigning — e.g. change the assignment in the
`_statements.html.erb` fragment where `statements =
account.account_statements.with_attached_original_file.ordered` to `statements =
account.account_statements.with_attached_original_file.ordered.to_a` and update
uses of `statements.count` to `statements.size` so the count/any?/each operate
on the already-loaded array rather than issuing extra SQL.
In `@test/controllers/accounts_controller_test.rb`:
- Around line 356-367: The duplicated uploaded_file helper (method uploaded_file
in accounts_controller_test.rb) should be extracted to a single shared helper
(e.g., define uploaded_file in test/test_helper.rb or a module under
test/support and include it into ActiveSupport::TestCase) so all tests use the
same implementation and return type (choose one:
ActionDispatch::Http::UploadedFile or Rack::Test::UploadedFile) and update the
three callers (accounts_controller_test.rb,
active_storage_authorization_test.rb, account_statements_controller_test.rb) to
remove their local definitions and rely on the shared uploaded_file helper;
ensure the shared helper creates the Tempfile, sets binmode, writes/rewinds
content, and returns the chosen uploaded file type so behavior is consistent
across suites.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53a3672d-642b-44b9-b8c5-2ed88f17ec96
📒 Files selected for processing (35)
.devcontainer/Dockerfile.devcontainer/docker-compose.ymlapp/components/UI/account_page.html.erbapp/components/UI/account_page.rbapp/controllers/account_statements_controller.rbapp/helpers/account_statements_helper.rbapp/helpers/settings_helper.rbapp/models/account.rbapp/models/account_statement.rbapp/models/account_statement/account_matcher.rbapp/models/account_statement/coverage.rbapp/models/account_statement/metadata_detector.rbapp/models/family.rbapp/views/account_statements/index.html.erbapp/views/account_statements/show.html.erbapp/views/accounts/show.html.erbapp/views/accounts/show/_menu.html.erbapp/views/accounts/show/_statements.html.erbapp/views/settings/_settings_nav.html.erbconfig/initializers/active_storage_authorization.rbconfig/locales/models/account_statement/en.ymlconfig/locales/views/account_statements/en.ymlconfig/locales/views/accounts/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260505120000_create_account_statements.rbdb/migrate/20260505130000_harden_account_statements.rbdb/migrate/20260505131000_validate_account_statement_constraints.rbdb/schema.rbtest/controllers/account_statements_controller_test.rbtest/controllers/accounts_controller_test.rbtest/fixtures/account_statements.ymltest/integration/active_storage_authorization_test.rbtest/models/account_statement_test.rbtest/system/settings_test.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
app/helpers/settings_helper.rb (1)
4-29: ⚡ Quick winAll other
SETTINGS_ORDERentries still use hard-coded English strings — consider migrating them to the same i18n lambda pattern.The new Statement Vault entry correctly wraps its label in
-> { t(...) }, which aligns with the project guideline of always usingt()for user-facing strings. The remaining 20+ entries still use plain string literals (e.g.,"Accounts","Bank Sync","Categories") and would not translate if i18n support were ever needed.♻️ Example migration for the first few entries
SETTINGS_ORDER = [ # General section - { name: "Accounts", path: :accounts_path }, + { name: -> { t("settings.settings_nav.accounts_label") }, path: :accounts_path }, { name: -> { t("settings.settings_nav.statement_vault_label") }, path: :account_statements_path, condition: :admin_user? }, - { name: "Bank Sync", path: :settings_bank_sync_path }, + { name: -> { t("settings.settings_nav.bank_sync_label") }, path: :settings_bank_sync_path }, # … and so on for each entry ]As per coding guidelines: "Always use
t()helper for user-facing strings."🤖 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 `@app/helpers/settings_helper.rb` around lines 4 - 29, The SETTINGS_ORDER entries use hard-coded English labels; update each name value (e.g., "Accounts", "Bank Sync", "Categories", etc.) to use a lambda that calls the i18n helper (-> { t("settings.settings_nav.<snake_case_key>") }) consistent with the existing Statement Vault entry so all user-facing strings are translatable; keep the same keys pattern as the Statement Vault (use unique keys per label) and leave path and condition symbols unchanged so SETTINGS_ORDER (in settings_helper.rb) behavior is unaffected.app/views/accounts/show/_statements.html.erb (1)
80-99: ⚡ Quick winPrecompute reconciliation balances for the table.
This loop calls
statement.reconciliation_statusper row, and each call can do twoaccount.balances.find_byqueries. The newbalance_lookup:hook in the model makes it easy to avoid turning this table into an N+1 hotspot.One way to remove the per-row queries
<% coverage = AccountStatement::Coverage.for_year(account, statement_year) %> <% statements = account.account_statements.with_attached_original_file.ordered.to_a %> +<% balance_records = account.balances + .where(date: statements.flat_map { |statement| [ statement.period_start_on, statement.period_end_on ] }.compact.uniq) + .index_by { |balance| [ balance.date, balance.currency ] } %> <% permission = account.permission_for(Current.user) %> <% can_manage_statements = AccountStatement.statement_manager?(Current.user) && permission.in?([ :owner, :full_control ]) %> @@ - <td class="px-3 py-3"> - <% case statement.reconciliation_status %> + <td class="px-3 py-3"> + <% reconciliation_status = statement.reconciliation_status( + balance_lookup: ->(date, currency) { balance_records[[date, currency]] } + ) %> + <% case reconciliation_status %>🤖 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 `@app/views/accounts/show/_statements.html.erb` around lines 80 - 99, The view is triggering N+1 queries by calling statement.reconciliation_status inside statements.each (which calls account.balances.find_by); fix by precomputing reconciliation info via the new balance_lookup hook in the controller (or presenter) and passing it into the view: call Account#balance_lookup (or the model method that returns the lookup/hash) once for the collection, build a map of statement_id -> reconciliation_status, and update the template to read from that map instead of calling statement.reconciliation_status for each row.app/views/account_statements/show.html.erb (2)
67-75: 💤 Low valueMinor: i18n key consistency for the "unknown" placeholder.
t(".institution_name_hint")etc. are scoped to this view, butt("account_statements.balance.unknown")reaches into a different namespace from inside the samedl. If the placeholder is reused elsewhere, this is fine; otherwise consider mirroring it under the local view scope (e.g.,.unknown_value) for easier maintenance.🤖 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 `@app/views/account_statements/show.html.erb` around lines 67 - 75, The placeholder translation used for missing values is pulled from a global key t("account_statements.balance.unknown") while the surrounding labels use view-scoped keys like t(".institution_name_hint"), t(".account_name_hint") and t(".account_last4_hint"); update the view to use a local scoped key (e.g., t(".unknown_value")) for the unknown placeholder and add the matching entry to this view's locale section so the unknown placeholder is maintained under the same namespace as the other keys (adjust `@statement.institution_name_hint`, `@statement.account_name_hint`, `@statement.account_last4_hint` usages to call t(".unknown_value") when empty).
50-50: 💤 Low valueConsider a constrained currency input for the editable form.
A free-form
text_field :currencylets users submit anything; the model relies on normalization to coerce values like"usd"→"USD". Aselectfrom a known currency list (e.g.,Money::Currency.tableor the app's existing currency list) would prevent typos and unsupported codes from reaching reconciliation/format paths. Not blocking — the controller normalizes — but worth tightening at the form layer.🤖 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 `@app/views/account_statements/show.html.erb` at line 50, The currency field currently uses a free-form text input (form.text_field :currency) which allows invalid or mistyped codes; replace it with a constrained select (e.g., use f.select or select_tag for :currency) populated from the canonical currency list (e.g., Money::Currency.table.keys or your app’s existing currency list) so users can only pick supported ISO codes; keep the existing controller/model normalization but change the view helper to f.select :currency, currency_options to limit inputs and display localized labels if needed.test/controllers/account_statements_controller_test.rb (4)
13-22: 💤 Low valueHardcoded English UI strings in assertions are fragile to i18n.
assert_select "h1", text: "Statement Vault"and the various hardcodedflash[:alert]/[:notice]literals (lines 22, 105, 135-136, 149, 166, 186, 224, 286, 394) will break the moment those strings move toconfig/locales/.... Asserting againstI18n.t("account_statements.index.title")(etc.) keeps the tests aligned with the localization-first guideline applied elsewhere in the PR.As per coding guidelines: "Always use
t()helper for user-facing strings."🤖 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 `@test/controllers/account_statements_controller_test.rb` around lines 13 - 22, Replace hardcoded UI strings in tests with the I18n helper: for assertions that currently use literal text (e.g., assert_select "h1", text: "Statement Vault") and flash checks like assert_equal "You don't have permission to manage this account", flash[:alert], change them to use the translation helper (t or I18n.t) keys (for example t("account_statements.index.title") and the appropriate flash translation key) so tests reference the same locale keys as the app; update all occurrences in this test file (including other flash[:alert]/[:notice] assertions) to use the corresponding I18n keys instead of hardcoded English strings.
397-409: ⚡ Quick winDuplicate
family_guesthelper across test files.A near-identical
family_guestprivate helper exists intest/integration/active_storage_authorization_test.rb(lines 162-174). Consider hoisting it intotest/test_helper.rb(or a small concern) so the role/seed data stays in sync as the User model evolves.🤖 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 `@test/controllers/account_statements_controller_test.rb` around lines 397 - 409, The two duplicate family_guest helpers should be hoisted into a single shared test helper: extract the family_guest method (currently memoized as `@family_guest` and that calls users(:family_admin).family.users.create! with first_name/last_name/email/password/role/onboarded_at/ui_layout) into test/test_helper.rb (or a small module/concern) and have both test/controllers/account_statements_controller_test.rb and test/integration/active_storage_authorization_test.rb call that shared family_guest helper instead of defining their own private methods; ensure the memoization variable name and attributes remain identical so existing tests keep behavior as User evolves, and remove the duplicated private method definitions from the test files.
170-187: 💤 Low valueTest allocates ~25MB on every run; consider stubbing
MAX_FILE_SIZE.
"x" * (AccountStatement::MAX_FILE_SIZE + 1)materializes a 25MB+1 string in memory each time this test runs. StubbingAccountStatement::MAX_FILE_SIZEto a small value (e.g., 16 bytes) would exercise the same branch with negligible overhead and keep CI snappy.♻️ Suggested change
test "rejects oversized statement upload" do + AccountStatement.stubs(:const_get).with(:MAX_FILE_SIZE).returns(16) + # or temporarily redefine the constant for this test assert_no_difference "AccountStatement.count" do post account_statements_url, params: { account_statement: { files: [ uploaded_file( filename: "oversized.csv", content_type: "text/csv", - content: "x" * (AccountStatement::MAX_FILE_SIZE + 1) + content: "x" * (AccountStatement::MAX_FILE_SIZE + 1) # adjust to small stubbed value ) ] } } end(In practice, redefining the constant via
silence_warnings { AccountStatement.const_set(:MAX_FILE_SIZE, 16) }inside asetup/teardownis the cleanest path; pick whichever convention the codebase prefers.)🤖 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 `@test/controllers/account_statements_controller_test.rb` around lines 170 - 187, The test "rejects oversized statement upload" currently builds a huge string using AccountStatement::MAX_FILE_SIZE; instead stub that constant to a small value during the test to avoid allocating ~25MB. In the test file wrap a temporary redefinition of AccountStatement::MAX_FILE_SIZE (use silence_warnings { AccountStatement.const_set(:MAX_FILE_SIZE, 16) } or your test helper's equivalent) in setup/teardown or around the specific test so the uploaded_file call uses the reduced size, then restore the original constant afterwards; ensure you reference the same test name and AccountStatement::MAX_FILE_SIZE when locating the change.
108-137: 💤 Low valueOptional: prefer mocha for the conditional stub to match the rest of the file.
The file already uses mocha for
AccountStatement.any_instance.stubs(:destroy)at line 387. Here you switch to Minitest's.stubwith a hand-rolled lambda +original_createto fall through. Mocha can express the same intent more directly via sequenced expectations (e.g.,AccountStatement.expects(:create_from_prepared_upload!).twicewith.raises(ActiveRecord::RecordInvalid.new(invalid_record)).then.returns(saved_record)), keeping the test consistent with the project's stated stubbing tool.As per coding guidelines: "Use the
mochagem for stubs and mocks".🤖 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 `@test/controllers/account_statements_controller_test.rb` around lines 108 - 137, The test currently uses Minitest's AccountStatement.stub with a custom lambda and original_create to simulate a first-call RecordInvalid then a success; replace this with a mocha sequenced expectation to match the rest of the file: use AccountStatement.expects(:create_from_prepared_upload!).twice (or the appropriate call count) and chain .raises(ActiveRecord::RecordInvalid.new(invalid_record)).then.returns(a saved AccountStatement instance) so the first invocation raises and the second returns a persisted record; also remove the original_create capture and the lambda; keep the existing invalid_record setup and the assertion expectations unchanged and ensure AccountStatement.any_instance.stubs(:destroy) remains as-is elsewhere for consistency.
🤖 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 `@app/models/account_statement.rb`:
- Around line 354-359: The sync_review_status callback currently returns only
when both rejected? and will_save_change_to_review_status? are true, so a
statement marked rejected can be overwritten on unrelated saves; change the
guard in sync_review_status to return when the record is rejected and the
review_status is not being changed (i.e. return if rejected? &&
!will_save_change_to_review_status?) so manual rejections are preserved across
unrelated updates but still allow intentional review_status changes; locate the
sync_review_status method and adjust the early-return condition accordingly
while keeping the subsequent assignments to self.review_status when
account.present?/account.blank? and linked?/linked? intact.
- Around line 100-114: The attach call to statement.original_file may leave a
staged blob if save! or the RecordNotUnique branch fails; wrap the create flow
so any exception (including in MetadataDetector.apply, statement.save! or the
rescue branch before re-raising) ensures the uploaded blob is removed (call
purge or purge_later on statement.original_file.blob if present). Update the
block around statement.original_file.attach / MetadataDetector.new(...).apply /
statement.save! and the rescue ActiveRecord::RecordNotUnique path to purge the
blob when a duplicate is not returned or any other error is raised, referencing
statement.original_file.attach, MetadataDetector, statement.save!, rescue
ActiveRecord::RecordNotUnique, duplicate_for and DuplicateUploadError to locate
where to add the cleanup.
---
Nitpick comments:
In `@app/helpers/settings_helper.rb`:
- Around line 4-29: The SETTINGS_ORDER entries use hard-coded English labels;
update each name value (e.g., "Accounts", "Bank Sync", "Categories", etc.) to
use a lambda that calls the i18n helper (-> {
t("settings.settings_nav.<snake_case_key>") }) consistent with the existing
Statement Vault entry so all user-facing strings are translatable; keep the same
keys pattern as the Statement Vault (use unique keys per label) and leave path
and condition symbols unchanged so SETTINGS_ORDER (in settings_helper.rb)
behavior is unaffected.
In `@app/views/account_statements/show.html.erb`:
- Around line 67-75: The placeholder translation used for missing values is
pulled from a global key t("account_statements.balance.unknown") while the
surrounding labels use view-scoped keys like t(".institution_name_hint"),
t(".account_name_hint") and t(".account_last4_hint"); update the view to use a
local scoped key (e.g., t(".unknown_value")) for the unknown placeholder and add
the matching entry to this view's locale section so the unknown placeholder is
maintained under the same namespace as the other keys (adjust
`@statement.institution_name_hint`, `@statement.account_name_hint`,
`@statement.account_last4_hint` usages to call t(".unknown_value") when empty).
- Line 50: The currency field currently uses a free-form text input
(form.text_field :currency) which allows invalid or mistyped codes; replace it
with a constrained select (e.g., use f.select or select_tag for :currency)
populated from the canonical currency list (e.g., Money::Currency.table.keys or
your app’s existing currency list) so users can only pick supported ISO codes;
keep the existing controller/model normalization but change the view helper to
f.select :currency, currency_options to limit inputs and display localized
labels if needed.
In `@app/views/accounts/show/_statements.html.erb`:
- Around line 80-99: The view is triggering N+1 queries by calling
statement.reconciliation_status inside statements.each (which calls
account.balances.find_by); fix by precomputing reconciliation info via the new
balance_lookup hook in the controller (or presenter) and passing it into the
view: call Account#balance_lookup (or the model method that returns the
lookup/hash) once for the collection, build a map of statement_id ->
reconciliation_status, and update the template to read from that map instead of
calling statement.reconciliation_status for each row.
In `@test/controllers/account_statements_controller_test.rb`:
- Around line 13-22: Replace hardcoded UI strings in tests with the I18n helper:
for assertions that currently use literal text (e.g., assert_select "h1", text:
"Statement Vault") and flash checks like assert_equal "You don't have permission
to manage this account", flash[:alert], change them to use the translation
helper (t or I18n.t) keys (for example t("account_statements.index.title") and
the appropriate flash translation key) so tests reference the same locale keys
as the app; update all occurrences in this test file (including other
flash[:alert]/[:notice] assertions) to use the corresponding I18n keys instead
of hardcoded English strings.
- Around line 397-409: The two duplicate family_guest helpers should be hoisted
into a single shared test helper: extract the family_guest method (currently
memoized as `@family_guest` and that calls
users(:family_admin).family.users.create! with
first_name/last_name/email/password/role/onboarded_at/ui_layout) into
test/test_helper.rb (or a small module/concern) and have both
test/controllers/account_statements_controller_test.rb and
test/integration/active_storage_authorization_test.rb call that shared
family_guest helper instead of defining their own private methods; ensure the
memoization variable name and attributes remain identical so existing tests keep
behavior as User evolves, and remove the duplicated private method definitions
from the test files.
- Around line 170-187: The test "rejects oversized statement upload" currently
builds a huge string using AccountStatement::MAX_FILE_SIZE; instead stub that
constant to a small value during the test to avoid allocating ~25MB. In the test
file wrap a temporary redefinition of AccountStatement::MAX_FILE_SIZE (use
silence_warnings { AccountStatement.const_set(:MAX_FILE_SIZE, 16) } or your test
helper's equivalent) in setup/teardown or around the specific test so the
uploaded_file call uses the reduced size, then restore the original constant
afterwards; ensure you reference the same test name and
AccountStatement::MAX_FILE_SIZE when locating the change.
- Around line 108-137: The test currently uses Minitest's AccountStatement.stub
with a custom lambda and original_create to simulate a first-call RecordInvalid
then a success; replace this with a mocha sequenced expectation to match the
rest of the file: use
AccountStatement.expects(:create_from_prepared_upload!).twice (or the
appropriate call count) and chain
.raises(ActiveRecord::RecordInvalid.new(invalid_record)).then.returns(a saved
AccountStatement instance) so the first invocation raises and the second returns
a persisted record; also remove the original_create capture and the lambda; keep
the existing invalid_record setup and the assertion expectations unchanged and
ensure AccountStatement.any_instance.stubs(:destroy) remains as-is elsewhere for
consistency.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89dc0134-b04a-4976-96ee-f10b7d211932
📒 Files selected for processing (14)
.devcontainer/Dockerfileapp/controllers/account_statements_controller.rbapp/helpers/account_statements_helper.rbapp/helpers/settings_helper.rbapp/models/account.rbapp/models/account_statement.rbapp/models/account_statement/metadata_detector.rbapp/views/account_statements/show.html.erbapp/views/accounts/show/_statements.html.erbtest/controllers/account_statements_controller_test.rbtest/controllers/accounts_controller_test.rbtest/integration/active_storage_authorization_test.rbtest/models/account_statement_test.rbtest/test_helper.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/account_statements_controller.rb
- app/models/account_statement/metadata_detector.rb
brin-security-scanner
Bot
added
contributor:verified
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/helpers/account_statements_helper.rb (1)
17-32: 💤 Low valueOptional: fold
duplicateandambiguousinto one branch.Both currently return the identical warning class string. Combining them keeps "warning-styled coverage states" co-located so a future palette tweak can't drift between the two.
♻️ Proposed refactor
def account_statement_coverage_classes(status) case status.to_s when "not_expected" "bg-container-inset text-subdued ring-alpha-black-25" when "covered" "bg-success/10 text-success ring-success/20" - when "duplicate" - "bg-warning/10 text-warning ring-warning/20" - when "ambiguous" + when "duplicate", "ambiguous" "bg-warning/10 text-warning ring-warning/20" when "mismatched" "bg-destructive/10 text-destructive ring-destructive/20" else "bg-gray-tint-5 text-secondary ring-alpha-black-50" end end🤖 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 `@app/helpers/account_statements_helper.rb` around lines 17 - 32, In account_statement_coverage_classes, fold the identical branches for statuses "duplicate" and "ambiguous" into a single when clause (e.g. when "duplicate", "ambiguous") so both map to the same warning class string; update the case in the account_statement_coverage_classes method to use one combined branch and remove the duplicate branch to keep warning-state styling co-located.app/models/account_statement.rb (1)
347-357: 💤 Low valueOptional: derive format predicates from
ALLOWED_EXTENSION_CONTENT_TYPES.The CSV content-type list at line 352 duplicates
ALLOWED_EXTENSION_CONTENT_TYPES[".csv"], and the PDF/XLSX checks duplicate the single entry from each constant slot. If the allow-list is ever extended (e.g. another CSV variant), these predicates can silently drift. Re-using the constant keeps the canonical mapping in one place.♻️ Proposed refactor
- def pdf? - content_type == "application/pdf" - end - - def csv? - content_type.in?(%w[text/csv text/plain application/csv application/vnd.ms-excel]) - end - - def xlsx? - content_type == "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" - end + def pdf? + ALLOWED_EXTENSION_CONTENT_TYPES.fetch(".pdf").include?(content_type) + end + + def csv? + ALLOWED_EXTENSION_CONTENT_TYPES.fetch(".csv").include?(content_type) + end + + def xlsx? + ALLOWED_EXTENSION_CONTENT_TYPES.fetch(".xlsx").include?(content_type) + end🤖 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 `@app/models/account_statement.rb` around lines 347 - 357, The predicate methods pdf?, csv? and xlsx? duplicate hard-coded MIME types; instead derive them from the canonical ALLOWED_EXTENSION_CONTENT_TYPES mapping so the predicates stay in sync. Update csv? to check content_type.in?(ALLOWED_EXTENSION_CONTENT_TYPES[".csv"]) and update pdf? and xlsx? to compare against ALLOWED_EXTENSION_CONTENT_TYPES[".pdf"] and ALLOWED_EXTENSION_CONTENT_TYPES[".xlsx"] (or use .in? where the mapping yields arrays) so all three methods reference ALLOWED_EXTENSION_CONTENT_TYPES rather than repeating literal strings.
🤖 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.
Nitpick comments:
In `@app/helpers/account_statements_helper.rb`:
- Around line 17-32: In account_statement_coverage_classes, fold the identical
branches for statuses "duplicate" and "ambiguous" into a single when clause
(e.g. when "duplicate", "ambiguous") so both map to the same warning class
string; update the case in the account_statement_coverage_classes method to use
one combined branch and remove the duplicate branch to keep warning-state
styling co-located.
In `@app/models/account_statement.rb`:
- Around line 347-357: The predicate methods pdf?, csv? and xlsx? duplicate
hard-coded MIME types; instead derive them from the canonical
ALLOWED_EXTENSION_CONTENT_TYPES mapping so the predicates stay in sync. Update
csv? to check content_type.in?(ALLOWED_EXTENSION_CONTENT_TYPES[".csv"]) and
update pdf? and xlsx? to compare against ALLOWED_EXTENSION_CONTENT_TYPES[".pdf"]
and ALLOWED_EXTENSION_CONTENT_TYPES[".xlsx"] (or use .in? where the mapping
yields arrays) so all three methods reference ALLOWED_EXTENSION_CONTENT_TYPES
rather than repeating literal strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3076bb46-2d2e-469b-ae24-2a27fbcdec7d
📒 Files selected for processing (12)
app/helpers/account_statements_helper.rbapp/helpers/settings_helper.rbapp/models/account_statement.rbapp/views/account_statements/show.html.erbapp/views/accounts/show/_statements.html.erbconfig/locales/views/account_statements/en.ymlconfig/locales/views/settings/en.ymlconfig/locales/views/settings/fr.ymltest/controllers/account_statements_controller_test.rbtest/integration/active_storage_authorization_test.rbtest/models/account_statement_test.rbtest/test_helper.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- config/locales/views/account_statements/en.yml
- app/views/account_statements/show.html.erb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/models/account_statement.rb`:
- Line 49: The :byte_size validation in AccountStatement allows 0-byte uploads
(numericality greater_than_or_equal_to: 0), so change the validation to reject
empty files (require > 0) or add an explicit presence/size check: update the
numericality on :byte_size to require greater_than: 0 (keeping
less_than_or_equal_to: MAX_FILE_SIZE) or add a custom validation that calls
something like validate_non_empty_upload; alternatively, enforce this earlier in
prepare_upload! by raising/adding errors when byte_size == 0 so empty CSV/XLSX
files cannot be persisted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2be36bee-3a99-4574-9638-5c2d40bd6ab1
📒 Files selected for processing (2)
app/helpers/account_statements_helper.rbapp/models/account_statement.rb
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
|
Small nit-pick: would move the "Statement Vault" option under "Transactions" in |
|
Merge from main received (May 6). Two open items from jjmata's review still need a response:
When these are addressed, please re-request review from @jjmata. Code review note from Claude Code Generated by Claude Code |
This has been resolved - let me know if any further changes are required! |
|
New commits pushed 2026-05-07. This is still awaiting a first human review from @jjmata. CodeRabbit has flagged 7 actionable comments across multiple rounds (e.g. Generated by Claude Code Generated by Claude Code |
JSONbored
force-pushed
the
codex/feat-account-statement-vault
branch
from
def23e6 to
05746d3
Compare
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
|
This shouldn't have been closed - investigating. Was fixing the merge conflict, seems GitHub's API is having some issues right now: 
|
|
Reopening - the option finally appeared to reopen, but still seems to be having issues. Looks like GitHub is having all sorts of issues right now, will try again in a bit. |
3 participants
Summary
Feature overview
This adds the core Statement Vault domain surface around
AccountStatement. Users can upload PDF, CSV, or XLSX statement files into a general unmatched inbox, or directly from an account's Statements tab. Sure stores the original file through ActiveStorage, records file metadata and checksums, detects conservative statement metadata where deterministic signals exist, suggests obvious account matches, and lets users manually link, unlink, reject, edit metadata, download, or delete statements.The account Statements tab shows month-by-month coverage states: covered, missing, duplicate, ambiguous, and mismatched. Reconciliation V1 is intentionally review-oriented: it compares statement opening/closing balances and derived period movement against Sure balance history when enough metadata exists, flags mismatches, and never mutates transactions.
What changed
AccountStatementwith family ownership, optional account linkage, suggested account linkage, ActiveStorage original-file attachment, metadata fields, parser confidence, match confidence, review status, sanitized parser output, MD5 checksum, and SHA-256 content digest..pdf,.csv, and.xlsx, including extension + detected MIME checks, PDF magic-header checks, 25MB file limit, and duplicate detection with DB-level race protection.Related prior work
Related: #1382
That earlier PR explored LLM-assisted PDF statement processing and reconciliation inside the PDF import flow. This PR does not merge that
PdfImportjob/UI behavior because V1 needs a smaller canonical foundation first: secure archival storage, account linking, deterministic metadata, coverage, and balance mismatch flags. AI/tool-calling and PDF transaction extraction can build onAccountStatementlater as review-before-import suggestions, but they should not be required for the vault and should not silently create or update transactions.Why
@jjmata please let me know your thoughts regarding this - happy to discuss if you disagree!
Screenshots
Captured locally with headless Chromium from the live devcontainer app. The files are in this Codex workspace and can be uploaded/attached when this draft is promoted for review:
Validation
docker compose -f .devcontainer/docker-compose.yml exec -T app bin/rails test test/models/account_statement_test.rb test/controllers/account_statements_controller_test.rb test/controllers/accounts_controller_test.rbdocker compose -f .devcontainer/docker-compose.yml exec -T app bin/rails test test/integration/active_storage_authorization_test.rb -n '/statement blob|linked statement|unmatched statement blob|orphaned statement|orphaned transaction|unauthenticated user/'docker compose -f .devcontainer/docker-compose.yml exec -T app bin/rubocop app/models/account_statement.rb test/models/account_statement_test.rb db/migrate/20260505130000_harden_account_statements.rb db/migrate/20260505131000_validate_account_statement_constraints.rbdocker compose -f .devcontainer/docker-compose.yml exec -T app bundle exec erb_lint app/views/account_statements/index.html.erb app/views/account_statements/show.html.erb app/views/accounts/show/_statements.html.erbdocker compose -f .devcontainer/docker-compose.yml exec -T app bin/brakemangit diff --checkcoderabbit review --agent -t committedNotes
Summary by CodeRabbit
New Features
Improvements
Tests