mirror of
https://github.com/we-promise/sure.git
synced 2026-09-05 14:51:15 +00:00
f78303ebbebfb6fb96a7d654701bfcb0bd048ed6
528
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Juan José MataandClaude
|
f78303ebbe |
Add function-calling probe to detect tool-use support (#3255)
* feat(ai-health): name the missing function calling behind an opaque chat error The assistant reads accounts, transactions and holdings through function calls, so every chat request carries a `tools` payload. A model without function-calling support rejects it — OpenRouter answers a bare 404 — and the operator sees only that status code, with nothing pointing at the model. Both earlier attempts at this guessed from the chat-time error; the AI status page already runs live probes, so let it answer the question directly instead. `AiHealth::Probe#function_calling` asks the configured model for one trivial tool call the way the assistant asks for its own: chat completions with `tools` for OpenAI-compatible endpoints, the Responses API for hosted OpenAI, and `messages.create` with `tools` for Anthropic, carrying the same strict schema `Provider::Openai` sends. Reading it against the plain LLM probe is what makes the verdict sound rather than a guess at 404s: plain chat passing while the same request with tools fails means the model has no function calling; a response that carries no tool call means the endpoint took the tools but the model ignored them; both failing, or a timeout, stays an ordinary probe failure. The AI status card gains a Function calling (tools) row, an alert naming the fix for each of the two bad outcomes, and a failure reason. The hosting settings model field now says the assistant needs a tools-capable model and links super admins to the check. Refs #830 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWCmRQ1ry26JnhA79s1ZKH * fix(ai-health): only call a refusal a refusal, and probe the route chat takes Two findings from review of the function-calling probe. A tools request can fail for reasons that say nothing about tool support: a 429, a 500, a dropped connection, an unreadable body. Reading any non-timeout failure as `:unsupported` sent the operator hunting for a new model over a transient blip. Only a 4xx the service chose to answer with — excluding the ones that mean "not now" or "not you" — is a refusal of the tools payload; everything else stays an ordinary probe failure. The bare 404 from OpenRouter that this page exists to explain still reads as missing function calling. `Provider::Openai#supports_responses_endpoint?` is the real routing decision and `OPENAI_SUPPORTS_RESPONSES_ENDPOINT` can flip it either way, so choosing the API from "is the endpoint custom" could probe Chat Completions while chat uses Responses, or the reverse — reporting on a path the assistant never takes. Ask the provider instead, and cache the two routes under separate keys. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWCmRQ1ry26JnhA79s1ZKH * fix(ai-health): confirm a tools refusal before blaming the model A client error on the tools request can mean "your tools payload" or "your request, tools or not" — an invalid schema, a route the endpoint does not serve, a model it will not run. Splitting those on the status code alone still put a 422 from an endpoint contract on the model's account and told the operator to go find another one. The probe now confirms it: when the tools request comes back a client error, it asks again with the tools taken off. Only if that lands is the tools payload what was turned down, and the probe says so with its own failure code — provider-agnostic, and no reading of error text for the word "tool", which would only ever fit the provider it was written against. Statuses that mean "not now" or "not you" (401, 402, 403, 408, 429) never get a second ask. `AiHealth` now just reports the probe's verdict instead of inferring one from the status. The troubleshooting fix no longer points at an OpenRouter free tier: those providers commonly log prompts and completions for training, and every assistant tool call carries accounts, transactions, and holdings. It points at the model recommendations already in this doc, and says why free tiers are the wrong place to look. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWCmRQ1ry26JnhA79s1ZKH --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Thomas SteiblandJohns
|
c6789a0fed |
fix(i18n): localize new chat default title (U9) (#3256)
Co-authored-by: Johns <19662585+Rowdy@users.noreply.github.com> |
||
|
Thomas SteiblandJohns
|
0a8f93cc2d |
fix: localize transaction selectors in German (U4) (#3254)
Co-authored-by: Johns <19662585+Rowdy@users.noreply.github.com> |
||
|
Thomas SteiblandJohns
|
f66053f83c |
fix(i18n): localize trade activity labels (U4) (#3282)
Co-authored-by: Johns <19662585+Rowdy@users.noreply.github.com> |
||
|
Thomas SteiblandJohns
|
9f7fd6fdef |
fix(i18n): localize SnapTrade device authorization (#3283)
Co-authored-by: Johns <19662585+Rowdy@users.noreply.github.com> |
||
|
|
bb835b9793 |
feat: Super Admins can Delete Users and Modify Families/Groups (#2868)
* Add admin family management features and tests
- Implement FamiliesController with destroy action to delete unused families.
- Add localization for success and error messages related to family deletion.
- Create FamiliesControllerTest to ensure proper functionality of family deletion.
- Update UserPolicyTest to include permissions for super admins to delete users.
- Enhance UsersControllerTest with tests for user family management, including moving users between families and creating new families.
* feat(users): enhance user management with family transfer validation and improved delete warnings
* Simplify user management actions column and combine family options
Move heavy user edit forms from table rows into a DS::Popover action
menu, add role badges to the user column, combine family migration and
creation inputs with a Stimulus controller, enable self-family
transfer for super admins, and add safety guards against demoting the
last super admin in the system.
* feat: add authentication type pills to admin user index to display SSO and local login status
* Add set password feature for local users in admin user management
- Add password field in action popover for users with local password login
- Enforce all registration password criteria (min 8 chars, mixed case, digit, special char)
- Block simultaneous family and password updates with clear error
- Show descriptive success notifications (role, password, both, family)
- Ignore password param for SSO-only users
- Add comprehensive tests for all password validation paths
* Resolve DS Drift Patrol findings and CI scan failures
- Wrap auth-type pills in DS::Tooltip instead of native title= attribute
- Add actions.manage_user key to locale and drop redundant default: fallbacks
- Fix RuboCop style offenses in Admin::UsersController
- Update Brakeman ignore entry fingerprint for Admin::UsersController#user_params
* fix: update badge query to target DS::Pill structure
* Fix DS::Tooltip misuse hiding SSO auth-type pill in admin users view
The SSO pill was passed as a block to DS::Tooltip, which caused it to
render inside the hidden div[role="tooltip"] instead of being visible.
The text: option ("SSO Provider: ...") was also silently ignored because
tooltip_content returns content (the block) over @text when a block is
given.
Fix: render the SSO/Local+SSO pill directly as visible content and pass
DS::Tooltip with no block so text: is used as the tooltip popup. An info
icon now appears next to the pill and shows the provider name on hover.
Fixes test: Admin::UsersControllerTest#test_index_renders_auth_type_pills_for_local_and_sso_users
* Remove redundant default: fallback from role pill i18n lookup
All admin.users.index.roles.{guest,member,admin,super_admin} keys are
defined in the locale file and used elsewhere in the same view without
a default:. The fallback was redundant for every valid role and would
silently mask a missing or renamed key instead of raising in
development.
Drop the default: user.role.humanize argument so that any future
missing key surfaces immediately as I18n::MissingTranslationData.
* Revert unrelated JS/schema/split churn; fix transfer_to_family! default role
- Revert 62 JS files (Biome formatter and unrelated controller changes)
- Revert db/schema.rb dump churn (no new migrations in this branch)
- Revert unrelated split transaction view changes (edit/new.html.erb)
- Fix User#transfer_to_family! role default: role: role evaluates to nil
when omitted; use explicit self.role to read model attribute
Keeps the PR focused on user/family management (~18-20 files).
* Fix last login and session count in admin user management
Store last_login_at and sessions_count directly on the users table
so they remain accurate after a user logs out.
- Add migration to add last_login_at (datetime) and sessions_count
(integer, default 0) columns to users, with backfill from sessions
- Add counter_cache: :sessions_count to Session#belongs_to :user so
the count auto-increments/decrements on session create/destroy
- Add after_create callback on Session to stamp user.last_login_at
- Update Admin::UsersController to read both values from users table
instead of aggregating Session rows (which disappear on logout)
* Fix user management PR pending CI items
* Keep test current session after sign in
* Address PR review comments for user transfers
* refactor: update user removal label to "Delete User" and standardize component attribute naming
* Address PR Review Feedback for User Management
* test: Fix families and users controller tests for user management PR
* Limit PR 2868 schema diff
* Fix PR 2868 user management CI failures
---------
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: sure-admin <sure-admin@splashblot.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
|
||
|
buzzromainandClaude Opus 5
|
7176b4b521 |
fix(goals): stop the first goal being told about earmarks it has none of (#3216)
* fix(goals): stop the first goal being told about earmarks it has none of Linking an account and leaving the amount blank shows "This account will fund whatever is left after the other earmarks". On a first goal there are no other earmarks — so the sentence points at something absent, and it is where a user meets the word for the first time. The row already carries `data-earmarked-by-others`, and it is zero in that case. With the account to itself the hint now says so plainly, and "earmark" appears only where earmarks actually exist — which lets the context do the explaining instead of the vocabulary needing it. Pinned server-side rather than in JS. The branch is a ternary; the failure that matters is the form not handing over the second string, which does not raise — the value reads as undefined and the line renders empty. There is also a test that the two hints stay different, since identical copy would leave the branch doing nothing and the first-time reader back where they started. Nothing runs `test/javascript` — no npm script, no CI step — so a test there would have guarded nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): tell an account claimed in full from one claimed by nobody Review on #3216. The new "this account is yours alone" hint keyed off `earmarked_by_other_goals`, which sums `allocated_amount` — and a whole-account link carries nil, so it contributes zero. An account another goal already claims in full therefore looked exactly like an unclaimed one. The form promised the whole balance, and `whole_account_link_must_be_exclusive` refused the blank allocation on submit. That is worse than the sentence this PR set out to fix: it does not merely describe something absent, it describes something the save then contradicts. The row carries both readings now, because neither can be derived from the other. `whole_account_claimed_by_other_goals?` asks the question the sum cannot answer, and the two share the same row filter so the goal being edited is excluded from both. The two tests added here also move above the first `private`, same as on `define_method`, which is public regardless — but they read as a mistake sitting among the helpers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
ff48acd04b |
feat(goals): offer recording a spend where the user is standing (#3215)
* feat(goals): offer recording a spend where the user is standing Adding money had a button on the goal page. Using it had none — the entry lived in the overflow menu, behind three conditions, and nowhere else. The asymmetry bit hardest at the one moment it mattered. "Record pledge" disappears once a goal is reached, so a user who hit the target and then spent some of it arrived at a page offering a single action: "Close this goal". That releases the earmark, and its own hint tells them to do it "once you have actually spent it" — asking for something the page gave them no way to say. The celebration panel now offers it beside closing, in that order, because that is the order the two happen in and closing is the one another click cannot undo. Same condition as the menu entry, which stays where it is: this is a second door, not a move. Beside, never instead. Plenty of goals are closed with nothing recorded, and the offer must not read as a step to clear first — a test pins that closing is still offered whenever it was before. The row is conditional rather than always rendered: a reserve gets neither action, and an empty flex div still carries its top margin, which would open a gap under copy that says there is nothing to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): hold the spend offer to accounts the reader can reach Review on #3215. `offer_recording_a_spend?` rode on `current_balance`, which counts every linked account — private ones included. A reader backed only by somebody else's private account was shown the link, then sent to a dialog with nothing to pick and a refusal on submit. The dialog has applied this scoping since #3176; the panel that points at it had not. It goes through `backing_within` now, on the reader's own accessible accounts. The component tests gained a session for the same reason: without a reader the offer is correctly withheld, so every assertion about it was measuring the wrong thing. Also from review: the reserve's empty-row test renders the component rather than only asking its predicates. Both could stay false while the template emitted the row anyway, which is precisely the gap that test exists for — it needed `ViewComponent::TestCase` to do it. The two page tests move above the first `private`. They did run where they were — Rails' `test` macro defines methods through `define_method` from a class method, which is public regardless of the surrounding visibility, and `-n` confirms Minitest picks them up — but tests wedged between private helpers read as a mistake whether or not they behave like one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): close the second door to the spend dialog Review on #3215. Scoping the offer to the reader's own accounts fixed the lifecycle panel and left the overflow menu on the old condition, so the two doors to the same dialog disagreed — and the older one still had the bug the newer one was written to avoid. A reader backed only by another member's private account was shown the menu entry, opened a dialog with nothing to pick, and was refused on submit. The question moves to the goal, where both doors ask it, and the reader's accounts come from the list the controller already builds for the dialog. That also removes the component's own broader lookup: it was plucking every accessible account on each goal-show render, duplicating work done upstream in the same request. It now takes the ids in, defaulting to none — a caller that forgets them withholds the offer, which is the safe way to be wrong about a permission. A controller test pins it page-wide, since the point is that neither door may offer it; putting the old condition back makes it fail. Also from review: the panel test claimed to be scoped to the panel while selecting `section`, which DS::Card emits for every card on the page. The action row has an id now and both tests use it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
3a4e92c6f8 |
feat(goals): call a reserve's amount what the rest of the app calls it (#3230)
* fix(goals): let a months-of-expenses reserve be created at all The mode could not be used from the UI. The form makes the amount field read-only in months mode — correctly, since the figure is derived — so the form submits it empty. `target_amount` is required and positive, and validations run before every save callback, so the derivation that fills it never got the chance. Creation came back 422 with "can't be blank" on a field the user is not allowed to type in. Reproduced through the controller before changing anything: response 422, no goal created. Every existing test set `target_amount` explicitly, which is why the model looked healthy — the gap was entirely on the path a user actually takes. The derivation moves to `before_validation`, where a derived value belongs: it is computed, then validated like any other. The dirty-state predicates change with it, since `will_save_change_to_*` describes a save that has not been decided on yet at that point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): move the new tests out of the private section Review flagged them as never running. They do — Rails' `test` macro goes through `define_method` from a class method, which defines a public method whatever the surrounding visibility, and `-n` confirms Minitest picks both up. Verified before touching anything: 2 runs, 7 assertions. Moved anyway. My insertion targeted the file's last `private` rather than its first, so they landed among the helper methods, where they read as a mistake whether or not they behave like one — three separate reviewers have now stopped on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): put the page tests where nobody has to check they run Review on #3229. The four page tests sat between `private` and a later `public`, and every reader so far has stopped to work out whether they run. They do — Rails' `test` macro calls `define_method` from a class method, and a method defined that way is public whatever the surrounding visibility — but a test whose behaviour has to be reasoned about is a test nobody trusts. They move above the first `private`, where the question does not come up. The second `private` goes with them: everything between it and the first was already private, so it did nothing. The fixed-amount test also gained the assertion it was missing. It named the guard it was protecting and then checked only the status, so a 422 arriving for any other reason would have kept it green. It now asserts the error the form actually puts in front of the user; flipping that paragraph's condition makes it fail, which is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * feat(goals): settle on target balance, the term this kind of app uses A reserve holds a balance rather than reaching an amount, and the page had three words for that one idea: "floor" twice, "level" seven times, and a field labelled "Target amount". The mode selector said "How the floor is set" three lines above a field called "Target amount". They are all replaced by **target balance** / **solde cible**. That is the term this kind of application uses, and it keeps the noun the rest of the page already leans on — "target" appears 55 times here. My first pass invented "Level to hold", which was internally tidy and standard nowhere: it fought 55 uses of a word that was not actually wrong. A target need not be a finish line; a target balance is one you hold. In months mode the balance also stops pretending to be a field. It is worked out from spending, so it is shown as a result with a line saying where it comes from — "Worked out when you save" on a goal that has none yet, and the balance it currently holds when editing one. That removes the `readOnly` toggle, which existed only to stop people typing into something that should not have been an input. Three smaller corrections while in the file: - `exceeds_earmark` had the actor backwards in both languages. The account does not earmark; the goal earmarks on the account. - The French `not_active` was a comma splice, where the file already uses a colon for that construction. - "se recomplète" is not standard French; a reserve "se reconstitue", and "ce qui lui manque" reads better than "son manque". A test asserts neither locale still says floor or niveau, so the three vocabularies cannot quietly come back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): stop a hidden field blocking the reserve it belongs to Replacing the read-only amount input with a hidden one left it `required`, and a required input is still validated by the browser while `display: none`. Submitting a months-based reserve was refused over a field the user could not see, and could not have filled in either — the reserve became impossible to create, which is the very thing the previous change set out to fix. Disabled rather than hidden, so it is barred from validation and its value stays out of the params, letting the derived figure land. The field also has to come back when there is nothing to derive from: with no spending history the model keeps whatever was typed, so the typed amount is then the only way to set a target at all. The form now asks the family that question up front and keeps the field where the answer is no. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): stop the label swap deleting the required marker Review on #3230. `_money_field` puts the required-field asterisk inside the label, in a span of its own. Swapping the wording with `textContent = label` replaces every child of that label, so the asterisk went with it — and `refresh()` runs on connect, so this fired on every goal form, one-off and fixed reserve included, not only the derived-months case this PR is about. Nothing put it back for the life of the page. Only the wording changes now: the label's text node is rewritten and the span left alone. A system test covers it, because nothing short of a browser can. It fails on the old code at the first assertion, before anything is clicked, which is where the bug actually landed. Also from review: the months derivation asked for the median twice, building an IncomeStatement each time. It reads it once now — the method stays un-memoized, which is what the refresh job needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
buzzromainandClaude Opus 5
|
4fbc0a4eb1 |
fix(reports): a transfer out is not a sale (#3192)
* fix(reports): a transfer out is not a sale A negative quantity is all it took to be counted as a sale. So moving an asset to another account you own — a transfer, a sweep, an exchange — was listed among the period's sales, and its cost basis was compared against that day's price to book a gain nobody made. Nothing was sold and nothing was realised. The labels for this already exist and are already trusted elsewhere: Transaction::INTERNAL_MOVEMENT_LABELS keeps the same four out of the income statement, for the same reason. The investment report just never consulted them. Two places learn to ask. Trade#realized_gain_loss returns nil for an internal movement, so no caller can book the gain; and the report's query leaves those trades out, so the movement is no longer counted or listed as a sale it never was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nN1GZi7Dv3d7yZUozw3Yo * fix(reports): keep a security exchange out of the internal-movement list Review on this PR. `Trade::INTERNAL_MOVEMENT_LABELS` aliased Transaction's, which holds "Exchange". On cash that means a currency exchange and really is internal. On a security the label covers "currency **or** security exchanges" — the repo's own guide says so — and a security-for-security exchange can dispose of an appreciated asset. So a labelled exchange dropped out of the sales report *and* `realized_gain_loss` returned nil for it. The gain did not move; it stopped existing. The two errors are not symmetrical, which is what decided this. Listing a movement that was not a sale is visible and correctable. Erasing a realized gain is neither — nothing on the page says a figure is missing. So the trade list keeps only the labels that unambiguously preserve ownership, and leaves the ambiguous one where the user can see it. Also from review: the test asked for `period: "last_30_days"`, but the controller reads `period_type`, so the request silently fell back to the current month and the trades dated three days earlier dropped out of range on the 1st to the 3rd. Confirmed with `travel_to Date.new(2026, 9, 2)`: fails on the old parameter, passes on the new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
e269d7f6f3 |
fix(goals): let a months-of-expenses reserve be created at all (#3229)
* fix(goals): let a months-of-expenses reserve be created at all The mode could not be used from the UI. The form makes the amount field read-only in months mode — correctly, since the figure is derived — so the form submits it empty. `target_amount` is required and positive, and validations run before every save callback, so the derivation that fills it never got the chance. Creation came back 422 with "can't be blank" on a field the user is not allowed to type in. Reproduced through the controller before changing anything: response 422, no goal created. Every existing test set `target_amount` explicitly, which is why the model looked healthy — the gap was entirely on the path a user actually takes. The derivation moves to `before_validation`, where a derived value belongs: it is computed, then validated like any other. The dirty-state predicates change with it, since `will_save_change_to_*` describes a save that has not been decided on yet at that point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): move the new tests out of the private section Review flagged them as never running. They do — Rails' `test` macro goes through `define_method` from a class method, which defines a public method whatever the surrounding visibility, and `-n` confirms Minitest picks both up. Verified before touching anything: 2 runs, 7 assertions. Moved anyway. My insertion targeted the file's last `private` rather than its first, so they landed among the helper methods, where they read as a mistake whether or not they behave like one — three separate reviewers have now stopped on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): put the page tests where nobody has to check they run Review on #3229. The four page tests sat between `private` and a later `public`, and every reader so far has stopped to work out whether they run. They do — Rails' `test` macro calls `define_method` from a class method, and a method defined that way is public whatever the surrounding visibility — but a test whose behaviour has to be reasoned about is a test nobody trusts. They move above the first `private`, where the question does not come up. The second `private` goes with them: everything between it and the first was already private, so it did nothing. The fixed-amount test also gained the assertion it was missing. It named the guard it was protecting and then checked only the status, so a 422 arriving for any other reason would have kept it green. It now asserts the error the form actually puts in front of the user; flipping that paragraph's condition makes it fail, which is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
e7619cac89 |
fix(goals): make the figure beside the ring agree with the ring (#3213)
* fix(goals): make the figure beside the ring agree with the ring Recording a spend left the goal page contradicting itself. The ring is drawn from `progress_percent`, which counts money still held plus money already spent on the goal; the figure beside it showed only the first half. A goal that had saved 5,000 and spent 2,000 of it rendered a 100% ring next to "3,000 of 5,000" — two answers on one card, with nothing to say which to believe. The model was never wrong: `progress_percent` and `remaining_amount` have both counted the two halves since the spend feature landed. Only the display took one of them. `progress_amount` names what progress actually counts, and both surfaces now read from it. The amount already used is reported as part of that total rather than beside it — "Including 2,000 already used" — so a reader has nothing to add up and no reason to read a completed goal as a shortfall. "Used" rather than "spent", matching the menu entry the user came through: it is the same gesture, and spending on the thing you saved for is the goal working, not failing. Shown only where there is something to show. The overwhelming majority of goals never record a spend, and a permanent "0 used" line would be noise on every card. A reserve refuses consumption outright, so this never appears on one. What is still sitting in each account keeps its place in the funding breakdown, which is where that question belongs — the view test asserts the headline specifically rather than the whole page, for exactly that reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): make the ring announce what it shows Review on #3213. The headline moved to the progress total; the ring's `aria-label` still read `current_balance_money`. A screen reader announced "$3,000 of $5,000 saved" while the line beside it said "$5,000, including $2,000 already used" — the same ring, two different numbers depending on whether you could see it. The wording moves with the figure. "Saved" stops being the whole truth once part of the total has been spent on the goal, so a goal that has recorded one gets the sentence that says so, and every other goal keeps the wording it had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * style(goals): use the component's t() for the ring's labels Review on #3213. `I18n.t` works, but the component helper is what the rest of the codebase reaches for and it carries the view's locale context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
db181eb84f |
fix(sparklines): prevent crash when trend is nil for empty series (#3171)
* fix(sparklines): prevent crash when trend is nil for empty series * Test sparklines without trend data --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
Tim Katz
|
5a798435b3 |
Fix user-triggered Plaid transaction refresh (#3206)
* Fix user-triggered Plaid transaction refresh Request a fresh Plaid institution update for explicit user syncs, then poll the saved cursor with bounded retries so private self-hosted instances do not depend on webhooks. Preserve the existing immediate sync and coalesce repeated refresh requests.\n\nCloses #3204 * Address Plaid refresh concurrency races * Preserve Plaid refresh handoff on retry exhaustion * Release Plaid refresh lease on enqueue errors Ensure adapter exceptions cannot leave the shared refresh cooldown occupied when no job was queued. |
||
|
Shibu M
|
683a3bf98d | Fix category merge form submission (#3225) | ||
|
|
695c1b4190 |
fix(transactions): Resolve N+1 query in bulk update controller (#3072)
* fix(transactions): Resolve N+1 query in bulk update controller * Test transaction bulk update eager loading --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
|
fb47e40247 |
Fix(lunchflow): Missing template on validation failure in create action (#2724)
* Fix(lunchflow): Missing template on validation failure in create action * Test invalid Lunchflow connection creation --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
Brandon
|
bf5ceff269 |
Fix flaky sign_out teardown in six test suites (#3208)
Six suites (passkey, MFA, SnapTrade, categorize, onboarding and the Active Storage authorization integration tests) share a sign_out helper that deletes the user's sessions through the controller, one HTTP request per session, iterating in unspecified order. The moment the loop deletes the session the test itself is signed in with, every later request in the loop is unauthenticated and silently deletes nothing, so whichever sessions happen to sort after it survive. The sessions fixture belongs to the same user these suites use, so a surviving fixture row then fails every assertion that expects the user to have no sessions. Row order usually favors the fixture, which is why the suites usually pass. Under parallel CI they fail a few times a week, always in this file family, always with the fixture session as the leftover. Forcing newest-first order reproduces it deterministically on current main: ten of the fifteen passkey tests fail. Teardown hygiene is not the behavior under test, so the helpers now destroy the sessions directly, which no order can break. All six suites run green three times in a row. |
||
|
Tim Katz
|
2fdb9ee175 |
Plaid: add accounts to an existing connection (#3199)
* feat: add accounts to existing Plaid items * fix: harden Plaid account addition * fix: guard Plaid follow-up sync retries * fix: use debug log for Plaid retry exhaustion |
||
|
Juan José Mata
|
2abce5f5b7 |
Verify PDF support with a synthetic health probe (#3191)
* Add synthetic PDF health checks * Require exact marker in PDF health probes * Report PDF health paths separately * Refactor application code * Remove unrelated schema dump changes * Simplify synthetic PDF validation --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
|
4930aacdb8 |
feat: create merchant inline from transaction detail (#3106)
* feat: create merchant inline from transaction detail Add a searchable "create or select" merchant combobox (DS::MerchantSelect, mirroring the existing DS::TagSelect pattern) so a new FamilyMerchant can be created directly from the transaction detail and new-transaction forms, instead of requiring a trip to Settings > Merchants first. FamilyMerchantsController#create now also responds to JSON so the combobox can create-and-select a merchant without a full page reload. * fix: address PR review feedback on merchant inline creation - Handle Turbo validation failures in FamilyMerchantsController#create (missing format.turbo_stream branch raised ActionController::UnknownFormat) - Guard connect() so disabled merchant selectors (no menu target) don't throw - Select the exact match (or block form submission) on Enter when the create row is hidden - Catch network/parse failures in createMerchant and show an error - Localize the fallback "could not create merchant" error message - Move the create button/error text outside the listbox and add aria-controls for accessibility - Align create-option avatar size with the selected-value display Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: keep created merchant option inside the listbox, move logo URL logic to component - Add a dedicated listbox target and insert newly created merchant options inside it (previously landed outside role="listbox" after the create button was moved out in the prior review-fix commit) - Move selected-merchant logo URL transformation out of the template into DS::MerchantSelect#selected_merchant_logo_url Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address maintainer review feedback on merchant inline creation - Drop the explicit format.turbo_stream branch in the merchant creation failure path: respond_to's turbo_stream matching forces the response Content-Type to text/vnd.turbo-stream.html before the block runs, so render :new, formats: [:html] only changed template lookup, not the Content-Type. Turbo's client then received a turbo-stream response with no <turbo-stream> tags and silently did nothing. Leaving turbo_stream undeclared lets Rails negotiate down to format.html, which renders :new with the correct text/html type. - Add truncate/shrink-0 classes to the merchant option partial's name and avatar spans so they match the selected-value display once updateSelectionDisplay clones them into the trigger button. - Add a regression test simulating a Turbo form submission (Accept: turbo-stream, text/html) against a duplicate merchant name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
03b783b139 |
feat(budgets): show what is actually free, beside the plan (#3179)
* feat(budgets): show what is actually free, beside the plan The budget has never consulted an account balance. `budgeted_spending` and `expected_income` are numbers the user typed, and every figure on the page derives from them — a forecast, checked against reality after the fact. It answers "what did I plan to spend" and cannot answer "what do I actually have". Three methods answer the second question: `available_cash`, `earmarked_for_goals`, and `free_cash`. They appear in their own panel, below the plan and outside it. That separation is the whole design, not a layout choice. Folding cash into the allocation arithmetic turns the budget into a different product — YNAB's, where you distribute money you hold rather than money you expect — and a page showing "expected income 3,000" beside "really free 1,600" leaves the reader unsure which number drives the split. `allocated_spending` and `available_to_allocate` keep their exact meaning; a test asserts none of them moves. **The subtraction has to be over the same accounts as the sum.** `Goal::FUNDABLE_ACCOUNT_TYPES` includes Investment, so a goal can be backed by a brokerage account that `available_cash` never counted. Subtracting that earmark would show a "really free" figure too low, or negative, with nothing on the page to explain it. `earmarked_for_goals` is therefore restricted to `cash_accounts`, and `Goal#backing_within` exists to ask that question. It reads through the shared pool rather than summing `allocated_amount`, because a whole-account link reserves no fixed slice: summed naively it counts as zero while actually claiming the remainder. Scoped like `#transactions` — a personal budget sees its owner's accounts, the household one what the viewer can see. A figure labelled "available" has to mean available to the person reading it. Behind the preview flag, because goals are: a panel that subtracts what they claim, and links to them, would otherwise point at a page the reader cannot open and explain a subtraction they cannot inspect. bin/rails test: 7083 runs, 28500 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(budgets): make the cash panel work in more than one currency Review on #3179. Three defects, all of them mine, all in the same seam. `ExchangeRate.find_rate` does not exist. Every multi-currency family opening the budget page hit a NoMethodError before the panel rendered. `find_or_fetch_rate` is the lookup the rest of the app uses. `earmarked_for_goals` summed each goal's backing in the goal's own currency and subtracted it from an `available_cash` that had been converted. A fully earmarked EUR 1,000 account in a USD budget read as 1,200 available, 1,000 earmarked and 200 free — when none of it is free. The French keys landed under `budget_categories` instead of `budgets`, so the partial's `t(".heading")` found nothing and French readers got the English fallback. A missing rate leaves the amount as it stands rather than raising: a panel wrong by the spread beats the whole budget page failing to render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
f0333c026e |
feat(goals): surface money that left a goal's accounts unexplained (#3177)
* feat(goals): surface money that left a goal's accounts unexplained Goals never read transactions. `current_balance` is a stock summed from account balances, so an outflow reaches a goal only as a smaller number, with nothing saying which goal it belonged to. `consume!` closes that gap, but only for a user who thinks to declare it — and the whole difficulty is that they have no reason to think of it. `Goal::WithdrawalDetector` surfaces the outflows nothing has claimed, and the goal page offers them: *if any of this was spent on Trip, say so*. One click records it with the transaction as evidence. This is the pull half of what `GoalPledge` does for money coming in. A pledge asks first and matches later; here there is nothing to promise, so the outflow is surfaced after the fact and attributed — or not. **Anchored on the transaction, not declared.** `consume!` now takes one and stamps `extra["goal"]["consumed_goal_id"]`, the same namespace the pledges write into. That is what makes attribution idempotent: replaying it cannot credit a goal twice for one spend, and the stamp happens inside the consumption's own transaction so a refusal rolls the whole thing back. **Sign matters more than it reads.** In Sure an inflow carries a NEGATIVE amount, so the detector selects the positive side. Reading it the other way round would have offered to attribute the user's deposits as spending, and the mistake would look right in a diff. A test pins it. **A reserve is excluded.** It is drawn down and refilled, not spent, and asking someone to attribute a withdrawal from one invites them to erase the very shortfall it exists to report. Known limitation, unchanged by this: `GoalPledge::Reconciler` only runs on provider imports, never on a hand-entered transaction. This detector reads entries directly and so has no such gap, but the two halves are not symmetric and that is worth knowing. bin/rails test: 7100 runs, 28540 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): only offer an outflow the goal could still have spent Review on #3177. The panel offered outflows for completed and archived goals. Those have handed their accounts back, so a later transaction on one is not evidence about this goal — attributing it writes spending into a history that is already closed. The detector now returns nothing for a released goal; `consume!` refuses these too, but the panel should not ask in the first place. Provisional transactions were offered as well. A pending charge can be reversed or replaced by its posted form, leaving the goal consumed for a transaction that no longer exists while the posted twin arrives unstamped and gets offered again. Filtered through the pending-provider SQL the rest of the app already uses. `thaw_completed_amount!` wiped `consumed_amount` unconditionally, so a goal that recorded a spend and was then archived straight from active lost that history on unarchive — and dropped its progress with it. Restarting is what clears the figure, and a direct archive never closed a lifecycle to restart from. Cleared now only when a frozen figure exists. The attribution button was a hand-rolled `button_to` with raw `btn` classes; it is `DS::Button` now, the same primitive the consumption dialog uses, so the two ways of recording a spend do not read as two features. Carried down from #3176 by rebase: the goal-level lock, the `:not_active` guard, and the success notice, which was blank on this path because the form posts only `transaction_id`. The resolved amount is formatted through `Money` and the account behind an attributed outflow now resolves through `accessible_accounts` like the named one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): keep a private backing account out of the outflow panel The same leak #3176 closed on the dialog, through a third door. A goal can be backed by an account private to another family member, and the panel listed its outflows — naming the account, what was spent on it and roughly its size to someone with no access to it. `WithdrawalDetector` takes `accounts:` now, and the controller passes the links narrowed to what the viewer may see. It defaults to every linked account for callers with no viewer to speak for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): use the shared separator key in the outflow panel DS Drift Patrol on #3177. The `·` between an outflow's date and its account was a bare literal. `shared.dot_separator` already exists and is already used three times in the goals views, so this was drift rather than a missing mechanism. Wrapped in `aria-hidden` like the existing uses: the separator is decorative, and a screen reader was reading it out between the two values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
b6029c1e28 |
feat(goals): show what each account still has room to earmark (#3166)
* feat(goals): show what each account still has room to earmark `Account#free_to_earmark` has existed, unused, since earmarks shipped — its own comment said the UI was a follow-up. This is that follow-up, and the wording is the substance of it. It does not say "over-allocated". `free_to_earmark` is negative for as long as the saving is unfinished, which is the normal condition of anyone with goals in progress: a 6,000 account backing two goals of 5,000 gives −4,000 and is a perfectly correct setup. A warning phrased as a fault would fire permanently and teach people to ignore it. The message states the consequence instead — the goals come to X for a balance of Y, so they progress pro rata — and is never styled as an error. The trap is the goal being edited. `goal_earmarked_total` counts every goal including that one, so reopening a goal that earmarks 5,000 on a 6,000 account shows 1,000 of headroom, and re-entering the same 5,000 trips a message about a setup the user has not touched. `earmarked_by_other_goals` excludes it, and only when it is persisted — a goal being created has nothing to exclude. The pool is read once per render and passed down, never per account: the form lists every fundable account the user can see. A test counts the query and fails at two. The Stimulus controller is its own, with 3 targets. goal_form_controller is at 10 against the 7 the project guidelines suggest, needs none of this state, and is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): read the typed amount strictly, and format it in the app's locale Addresses review feedback on #3166. `Number.parseFloat` accepts prefixes, so "500abc" became 500, and the bare comma-to-dot swap turned a thousands-separated "1,500" into 1.5. Either way the preview described an amount the user had not typed — and the second case is a habit from another locale, not a typo, so it would have gone unnoticed. The value now has to match a complete number before anything is computed. `Intl.NumberFormat(undefined, ...)` let the BROWSER pick the locale, so a French user on an English-locale browser read separators and symbol placement matching nothing else on the page. The amounts cannot be formatted server-side — they change with every keystroke — so the server passes `I18n.locale` and the client applies it. That puts the decision where the rest of the app's formatting already lives. bin/rails test: 6954 runs, 0 failures. RuboCop, erb_lint and biome clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): let the assistant create a second goal on a claimed account Review on #3166. The function always built whole-account links and had no way to express an earmark, so once exclusivity landed, asking for a second goal on an account another goal already claimed came back as a bare `validation_failed` — while the account list still advertised the account as available. A common request became an unexplained refusal. Three changes, and the list is the important one: it now says what is left on each account and which are claimed in full, because the assistant reasons from that list and had no way to know otherwise. `earmarks` is an optional map of account name to amount, so the assistant can reserve a slice rather than the whole balance. Accounts left out keep the previous behaviour and take whatever is spare. The refusal is named before the save — `account_claimed_in_full`, with the account names — so the assistant gets a reason it can act on and ask about, rather than a validation message it can only relay. Checked after the currency check, which is the more fundamental of the two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): move the spend tests back out of the private section The merge of `main` into this branch landed #3176's tests between `count_pool_queries` and the helpers below it, inside the `private` section and at the wrong indentation. `ci / lint` has been failing on `Layout/IndentationConsistency` since. They still ran — `test` is a class method, so `private` does not hide them — which is why the unit job stayed green while lint went red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
buzzromainandClaude Opus 5
|
d7bf401cc7 |
fix(onchain-wallets): call transfers transfers, and drop the address swap (#3153)
* fix(onchain-wallets): call transfers transfers, and drop the address swap Three things reported from real use. **A transfer was announced as a purchase.** A movement was imported with `activity_label: "Buy"/"Sell"` and named "Buy 1.5 FAKE" — but coins arriving at an address were not bought there, and nothing here knows whether they were ever bought at all. The trade shape stays, because it is what carries quantity and cost basis in this ledger, but the label is now "Transfer" and the name is the one the movement already had while it was unpriced. The old wording also made the same event rename itself the day a price turned up for it. Worth pairing with the change to trades/_header.html.erb, which until now read the amount's sign and would still say "Buy" whatever the label. **Changing a tracked address is gone.** It repointed the rows at a new address while keeping their accounts, holdings and history — so trades reconstructed from address A stayed under an account presented as address B. Its own help text said so out loud: "The accounts, holdings and history stay as they are." Removing the address and adding the new one is not just simpler, it is the only one of the two that is honest, because it takes the old history with the old address. **The buttons follow the app's conventions now.** Actions that repeat per row belong in a menu here — accounts/_account.html.erb renders its own that way — not in a row of labelled buttons, which is what a provider panel does when it has a single connection to act on. So the per-address actions are a menu, the per-asset disconnect is icon-only, and the accounts-page card gains the actions menu every other provider card already had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(onchain-wallets): give a tracked asset its icon on the accounts page Account#logo_url asks its provider adapter for one, and ours answered nothing: there is no institution behind a self-custody wallet, and nothing attaches a file, so every tracked asset showed a blank where every other account shows an icon. Fixing Security#crypto_base_asset covered the holdings list, which reads the security directly — this is the other path, and it went through the adapter. Built from the symbol rather than looked up. The accounts page renders one of these per account, so resolving a Security each would be a query per row, and the symbol is all Brandfetch's crypto endpoint needs. It answers nil without a client id, which is the same nothing the page shows today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(onchain-wallets): follow the moved banner and the menu in the browser The system suite still expected the settings panel to carry the read-only reassurance, and to find "Review tokens" as a visible button. Both moved in the previous commit: the banner into the linking modal, where the question it answers is actually asked, and the per-address actions into a menu, as repeated row actions are rendered everywhere else in this app. Caught by CI rather than by me — the per-branch checks I ran covered `bin/rails test` and not `test:system`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(onchain-wallets): pin the reassurance to the frame it moved into The assertion proved the banner was somewhere on the page, which is exactly what the change does not claim: the point is where it lives. Now it asserts the text is absent from the settings panel and present inside the modal, so the test fails if the banner drifts back or never arrives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(onchain-wallets): rename movements imported before transfers had a name Review on #3153. Wallets synced before this change keep their `Buy`/`Sell` labels and their "Buy 1.5 shares of CRYPTO:BTC" wording, and nothing was rewriting them: `perform_sync` returns early when no address changed on chain, and the repair pass only ever looked at display-only `Transaction` rows. A cold address would have shown the old wording indefinitely — which for a wallet nobody touches is most of them. The repair now relabels this processor's own trades too, scoped to its `external_id` prefix and to `source: SOURCE` so a trade the user entered by hand is never renamed. It runs from `perform_post_sync`, which is the pass that already runs for every linked asset rather than only the changed ones. Idempotent, so a nightly sync does not rewrite the same rows forever. Separately: `Security.brandfetch_crypto_url` interpolated the symbol straight into a URL path, and `Onchain::AssetSymbol.canonical` only upcases and trims. An on-chain token can be called whatever its deployer chose, so a slash pointed the path elsewhere on the CDN and a hash pushed the client id into a fragment Brandfetch never sees. Guarded in the helper rather than at the call site — six callers reach it from four providers. Also from review: the icon test saves and restores `Setting.brand_fetch_client_id` instead of hard-coding nil in its `ensure`, which was erasing whatever the suite had configured. The "one query" claim in a repair test's name was never asserted, and this change adds a second query. Renamed to what it actually checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(onchain): drop the last change_address test with its feature #3182 added a `change_address` test while this branch was removing the feature it exercises. The rebase kept both, leaving a test calling a route this branch deletes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
613964529a |
feat(goals): let a goal be spent without looking like it fell behind (#3176)
* feat(goals): let a goal be spent without looking like it fell behind Coming home from the holiday a goal paid for dropped it from 100% to 20%. The money went where it was meant to go, and the app read that as failure. The only way back was to edit the target — falsifying what the user had actually set out to save. `consumed_amount` records what was spent ON the thing the goal was for, and progress reads `(backing + consumed) / target`. Spending the money is no longer indistinguishable from losing it. **The part that is easy to miss.** `consume!` also shrinks the earmark on the account by the same amount. Without that, money the user has already spent stays reserved and keeps its share away from every sibling goal — the exact double-counting the exclusivity rules exist to prevent, arriving through the back door. A test pins it through the pro-rata haircut, where the effect is visible: a sibling's backing grows as the spent share is released. **Kept separate from `completed_amount`.** That one freezes the BACKING at closure; folding consumption into it would count the same money twice on a goal partly spent and then closed. A test asserts each side is counted once. **A reserve refuses consumption outright.** It is drawn down and refilled, not spent, and recording a withdrawal as consumption would erase the shortfall the reserve exists to report. `account:` may be omitted only when the goal has one link — with several, guessing would silently pick a side. The controller refuses an account id that resolves to nothing rather than falling back to nil, which on a single-link goal would have recorded the spend against an account the user never named. The write is its own action rather than a verb branch inside `consume`: HEAD routes like GET but `request.get?` is false for it, so a branch would send a HEAD request down the write path. Brakeman caught that. bin/rails test: 7088 runs, 28510 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): make a spend either happen entirely or not at all Review on #3176 found the recorded spend and the released earmark could drift apart, and that nothing stopped the two figures being edited out of agreement afterwards. The lock was on the link, not the goal. `consumed_amount` lives on the goal, so two concurrent requests locking only their own links both read the same old value, both passed the target check, and both added to it. The whole check-and-write now runs under `with_lock` on the goal. Consuming more than the chosen link held was silently clamped: the link released what it had while `consumed_amount` took the full figure, so money counted as spent stayed reserved against every sibling goal. It is refused now — `:exceeds_earmark` — rather than half-applied. A dialog left open in another tab could still post to a goal that had since been completed or archived; `:not_active` closes that. Two validations stop the pair being separated after the fact: a goal that has recorded a spend cannot become a reserve (reserves refuse consumption, so the figure would count toward progress on an object whose model treats spending as a shortfall), and the target cannot be lowered below what was already spent. Consuming cleared the columns and left the memos standing, so an instance that had already read its backing kept reporting the pre-spend figure. Progress holding steady is the feature — the earmark shrinks by what consumption grows by — which is exactly what hid the stale backing. Also from review: `accessible_accounts` rather than the whole family for the account picker, `DS::Select` rather than a bare `select_tag`, the flash amount through `Money#format`, and the French label for the menu entry, which I had left untranslated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): reuse the cache reset the class already owns Review pointed out `reset_state_dependent_caches!` exists for exactly this, and that hand-rolling a second ivar list was the wrong shape. It was also wrong in substance: mine omitted `@pooled_allocations`, and consuming shrinks a link's allocation, which is precisely what the pool is computed from. One list, kept in one place, stays right when a memo is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * docs(goals): put four comments back on the methods they describe Rebases had stranded them: a paragraph about clearing memos on an AASM transition, two about what reopening does to a frozen figure, and one about `reload` leaving memos standing had all piled up in front of `consumption_link_for`, which does none of those things. The last is dropped rather than moved — its explanation now sits at the call site in `consume!`, where the reset actually happens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): keep a private backing account out of the consume dialog A goal can be backed by an account the viewer is not allowed to see, and the first pass only guarded the named-account path. Two ways round it remained. The dialog listed every link, so it named private accounts outright. And with `account_id` left blank the model picked the sole link on its own, without anyone having checked the viewer could reach it — so a direct POST reduced a private account's earmark, the figures moving afterwards saying roughly how much was in it. The controller now derives the eligible links from `Current.user.accessible_accounts`, the dialog renders those, and a blank id resolves only to a sole *eligible* account. With none the request is refused; with several it stays nil and the model asks, as before. Naming the account explicitly matters even when the goal has several links: the one the viewer can reach is not necessarily the one the model would have picked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
2c0d262cf6 |
fix(onchain): a deleted account leaves no wallet row behind (#3182)
* fix(onchain): a deleted account leaves no wallet row behind Deleting a Sure account left its on-chain tracking row in place. The row had a callback to follow its account link into the grave, but the guard that stopped it destroying itself could not tell apart the two ways that link dies — by the row, or by the account — and so caught both. What survived was worse than untidy. The row synced nothing and showed nowhere, yet it still answered "yes" to "is this address already tracked?" and still held the asset's slot in the partial unique index. The address became unusable: adding it again was refused as a duplicate of something the user could not see, and moving another address onto it raised a database error. So three places learn the difference between a row that tracks an account and one that merely exists: the guard now skips only its own row's destruction, the family's linked check asks for a live link, and the linker and address change let a dead row make way instead of colliding with it. No migration: a row already orphaned on a running instance is absorbed the next time that address is tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nN1GZi7Dv3d7yZUozw3Yo * fix(onchain): stop an orphan row passing itself off as a tracked asset Review on #3182. Both findings are the same root: a row left behind by a deleted account has no Account and no AccountProvider, but nothing downstream was asking. `revise` counted any matching row as tracked, orphans included, so ticking an asset whose account had been deleted did nothing at all — no Account created, the row still orphaned, and the screen still reporting the asset as tracked. It now looks only at rows that are actually linked, and at the ones that survived the removal pass. `link` cleared only the rows matching the assets picked in that submit, so relinking a subset left the rest as orphans. They still show up in `grouped_accounts` and token review as tracked, and — before the fix above — they also stopped `revise` from ever rebuilding them. Reclaiming an address now clears every row it left behind. A live row is untouched, with a test that fails if the sweep widens. One existing test had to change its premise rather than its assertion. It built two unlinked rows and asserted the surviving row object was the same one; unlinked rows are orphans, so revising now rebuilds them and the survivor is a new row. Linking them first is what a tracked asset actually is, and the assertion then measures the removal it was written for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
6d7ca3584c |
feat(goals): reserves you maintain, not goals you finish (#3167)
* feat(goals): reserves you maintain, not goals you finish An emergency fund is not a goal you reach and close — it is a level you hold, and every withdrawal is a shortfall to make good. Sure treated it like anything else: at 100% it offered to close it, which would release the very money being set aside; a withdrawal dropped the bar with no sign that anything was owed. `kind` (added by the lifecycle lot without behavior) now means something. A maintained goal is `funded` or `depleted`, never `reached` — sitting at its floor is a steady state, not an achievement to file away. `complete` is refused by an AASM guard rather than merely hidden, so no path can release a reserve's earmark. Two ordering traps, both of which would have made a drained reserve invisible: `ACTIVE_DISPLAY_STATUS_RANK` falls back to 4 for any status it does not know, so an unranked `:depleted` would sort a drained emergency fund below everything else — the exact opposite of what it means. It ranks alongside `:behind` now, and `:funded` sorts near the end with the goals that need nothing. `behind_pace?` excludes reserves. `monthly_target_amount` and `pace` both derive from `target_date`, which a reserve does not have, so "save X/month to catch up" would be advice about a deadline that does not exist. The form leads with the choice, since it changes what the rest of it means, and hides the target date for a reserve rather than disabling it — a hidden field cannot submit a stale value that would then drive a pace. The card states the shortfall, which is exactly `remaining_amount`. The panel that offers a one-off its closing action tells a reserve it is intact and offers nothing, because there is nothing to do. Scope: fixed targets only. Targets expressed in months of expenses, the monthly refresh job, and the depletion insight are the next two PRs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): let a reserve behave like one everywhere it is shown Addresses review feedback on #3167. The kind selector never hid the target date. `data-controller="goal-kind"` sat on the selector div while its `dateField` target is a sibling, so `dateFieldTargets` came back empty and picking "Reserve to maintain" left the deadline on screen and submittable. The controller moves to the form wrapper, which encloses both. Hiding a field is not enforcement, so the model now clears `target_date` for a maintained goal. Normalising rather than rejecting: the field is hidden, and an error about something the user cannot see is not actionable. A date could only arrive through a conversion or a crafted request, and either way a stored deadline would drive a pace the reserve does not have. A completed goal could be switched to `maintained` from the edit form. It then sat in a released state — one that has handed its earmark back — while the show page promised its money stays reserved, and `complete` for reserves is refused precisely to prevent that state. `kind` is now locked while released: reopen first. Reserves counted against the "goals on track" tile. Their statuses are `funded`/`depleted`, which match none of the exclusions in `tracked_total`, so they could never reach the numerator and a family with one reserve read "0 of 1 on track" for a goal working exactly as intended. Two more places still spoke of pace to something that has none. `pace_line` is suppressed for reserves on the card, and a depleted reserve gets its own panel before the projection card — the projection's summary, catch-up line and colour are all built from a deadline. What a drained reserve needs is the number the projection cannot show: how much is missing from the floor. The French celebration copy read "Votre réserve est à son niveau", which never says which level. Each guard was confirmed load-bearing by removing it and watching its test fail. bin/rails test: 6962 runs, 28003 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Left open deliberately: extracting the show page's lifecycle panel into a ViewComponent. The guideline behind it is right, but the refactor is wider than this round of fixes and belongs on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): finish teaching the status consumers about reserves Second round of review feedback on #3167: two consumers still had no branch for the reserve statuses. `ProgressRingComponent#percent_text_class` styled only `:reached` as success, so a funded reserve — a floor the user is holding exactly as intended — fell back to the neutral colour and read as unfinished. `status_callout_context` had no `:depleted` branch, so a drained reserve showed no callout at all: the one status that most deserves a line of explanation was the only one saying nothing. It now names the shortfall. `:funded` deliberately keeps no callout — a reserve at its level has nothing to report, and the celebration panel already says so. A test pins that, so the silence reads as a decision rather than another missing branch. bin/rails test: 6964 runs, 28008 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): lock the kind on the state the goal is actually in Addresses review feedback on #3167, on the guard that landed in c1c7f4f7. `kind_locked_while_released` read the in-memory `state`, so a single write setting `state: "active"` alongside the new kind saw the goal as already reopened and waved it through. The end state looks legitimate — active and maintained — which is why the hole is easy to miss. It is not: the direct write skipped the `reopen` transition, and with it `thaw_completed_amount!`. `completed_amount` survived, so `current_balance` returned that frozen snapshot forever on a live reserve. Reopening has to be its own gesture, because it is the gesture that thaws. Now reads `state_in_database`, with a regression test on the combined write asserting both that it is refused and that the frozen amount is untouched. Confirmed load-bearing by reading the attribute again and watching it fail. bin/rails test: 6969 runs, 28019 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): send an empty reserve to the shortfall panel, not the empty state Addresses the last review thread on #3167. The `maintained?` branch sat after the zero-balance/zero-pace one, so a brand-new reserve matched the generic "make your first transfer" card. I had put it there on purpose, thinking a reserve with nothing in it wanted the first-transfer nudge. The review is right that it does not: it is still a reserve short of its floor, and the shortfall panel says so with the saved, target and missing amounts, where the generic card says none of them. Ordering it after also meant evaluating `pace` on a goal that has no pace to evaluate. bin/rails test: 6965 runs, 0 failures. RuboCop and erb_lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): stop a paused goal outranking a reserve that is whole Review on #3175. `:funded` and paused both ranked 3 in `active_display_sort`, so the tie broke on name and a paused goal called "Alpha" sat above a reserve called "Zeta" that was fully funded — the list saying the paused one wanted attention more. Paused now ranks behind every status, which is what the comment above the table already claimed. The seven panels on the goal page were hand-rolled repetitions of `DS::Card`'s exact shell, two of them adjacent and identical. They render through the primitive now, so their surface styling cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): move the lifecycle panel decision out of the template Review on #3167 and #3180. Which panel a goal gets is a lifecycle question with five answers, and the template worked it out inline from `completed?`, `maintained?`, `one_off?`, `status` and `may_complete?` — five predicates deep in ERB where the ordering between them was load-bearing and nothing said so. `Goals::LifecyclePanelComponent` answers it in Ruby and the template renders the answer. The markup moves across unchanged, keys made absolute because a relative `t(".x")` in a component resolves against the component's own path rather than the page these strings belong to. The order is now stated once, where it can be read and tested: `:reserve_shortfall` before `:empty`, because a brand-new reserve sits at zero balance and zero pace and the generic "make your first transfer" card would otherwise swallow it. Closing from the panel now confirms, as the header menu already did. Completing releases the goal's earmarked money, and the panel offered that in one click. Both go through `goal_complete_confirm` rather than building the wording twice — two copies drifting apart is how one ends up describing the wrong consequence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): refresh the pace suggestion when the deadline is cleared Assigning `input.value = ""` fires no event, so `goal-form#suggestedChanged` never ran: selecting "Reserve to maintain" cleared the date but left the monthly pace suggestion on screen, derived from a deadline the goal no longer has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): let a depleted reserve look as urgent as it is Review on #3179 and #3180. `Goal#needs_attention?` names the pair of statuses that mean "this one wants looking at" — a goal off its pace and a reserve below its floor. Three places were spelling that out and the Plan hub's progress bar had fallen behind, so a depleted reserve got a neutral bar an inch from its own amber status pill: the same goal reported as needing attention and not. `projection_summary` told a funded reserve it had "hit the target, no projection needed". A reserve holds a level; there is no finish line to project toward and no target to have hit. It does not reach that panel today — the shortfall and celebration panels catch it first — but the method reads as the single source of truth for that subtitle and should not hand a caller a one-off's wording. The legend swatches are bordered spans now rather than inline SVG, and the label takes `text-xs` instead of an arbitrary 11px. The projection swatch keeps the chart's own colour variables in an inline style rather than `border-success` / `border-warning`: the chart hard-codes green-600 and yellow-600, and a legend whose colour does not match the line it describes is worse than the markup it would save. The Plan-card test counts warning bars rather than matching one. A fixture goal is already off its pace, so the markup is on the page either way and a presence check passed without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
eb498cfa0c |
Feature/category hierarchy and account search (#2845)
* Add parent/child category hierarchy to all category selects; add search to account select
Category selection consistency:
- DS::Select (shared component powering the main transaction form,
transaction edit, bulk-update, and transfer category pickers) now
indents subcategories with a corner-down-right icon, matching the
existing transaction-row category dropdown.
- Feed DS::Select-based category pickers with Category.alphabetically_by_hierarchy
(parent name, then parent-before-children, then own name) so children
render directly under their parent.
- Added Category::Group.select_options, a shared helper producing
parent-then-child ordered options (with an indent marker) for plain
HTML <select> elements. Used by:
- Rule builder category condition/action selects
- Bulk 'categorize transactions' select
- CSV/QIF import category mapping select
- Grouped the splits category combobox and the transaction search
category filter checklist the same way, both with the
corner-down-right indent icon used elsewhere.
Account selection:
- Added searchable: true to the account select in the new/edit
transaction form, matching the category and merchant selects next
to it.
* Fix SyntaxError: 'for' is a Ruby reserved keyword
Category::Group.select_options called for(categories) as a bare method
call, but Ruby parses a bare 'for' as the start of a for..in loop
statement, not a method invocation. Qualify it as self.for(categories)
to call the class method explicitly.
Verified with 'ruby -c' on all touched .rb files and ERB.new(...).src
on all touched .erb files.
* Add test coverage for category hierarchy and account search
Model-level:
- Category::GroupTest (new): for() grouping and the new select_options
helper (order + indent labels).
- CategoryTest: alphabetically_by_hierarchy scope ordering.
- Rule::ConditionFilter::TransactionCategoryTest (new)
- Rule::ActionExecutor::SetTransactionCategoryTest (new)
- Import::CategoryMappingTest (new): grouping + 'Add as new category'
still prepends correctly.
Controller/integration-level (asserting actual rendered HTML order):
- SplitsControllerTest: category combobox data-value ordering.
- Transactions::CategorizesControllerTest: bulk-categorize <select>
option ordering.
- TransactionsControllerTest:
- search filter checkbox ordering (q[categories][])
- new-transaction DS::Select category ordering (via trigger id +
ancestor traversal)
- new-transaction account select renders a search box
All new/modified test files verified with 'ruby -c' (syntax) and
cross-checked fixture names, family scoping, route helpers, and field
names against the actual fixtures/routes/views. Ruby/Bundler network
access to rubygems.org is unavailable in this sandbox, so the suite
itself has not been executed — run 'bin/rails test' before merging.
* Align with design-sure conventions: keep domain logic in component, not template
Per .cursor/rules/view_conventions.mdc ('keep domain logic out of the
views'), the parent/child hierarchy check for DS::Select items belongs
in the component class, not inline in the ERB template. DS::Select
already has this exact pattern for other per-item derived properties
(color_for, icon_for, logo_for) — added child? alongside them and
updated the template to call it instead of computing it inline.
Added test/components/DS/select_test.rb (ViewComponent::TestCase,
no rendering needed) covering child? directly: subcategory objects,
root-category objects, non-hierarchical objects (merchants), and the
include_blank placeholder item.
Also did a broader pass against the design-sure .cursor/rules to confirm
the rest of this branch's changes already comply:
- Uses Current.family (never current_family) throughout
- Uses the icon() helper exclusively, never lucide_icon directly
- No new/hardcoded colors; only existing semantic Tailwind tokens
already used elsewhere in these same files
- No changes to sure-design-system.css / application.css
- Extended existing components/partials rather than creating new ones
where one already existed (view_conventions.mdc component-vs-partial
guidance)
- Test additions stay in Minitest + fixtures, avoid system tests,
and test query-method output directly (testing.mdc)
* Address CodeRabbit review: sort category groups, tighten test assertion, move grouping out of view
* Address review: fix arrow leak in rule summaries, filter panel alignment, simplify splits ordering
* fix(pages): set breadcrumbs for changelog and feedback pages (#2889)
* fix(app): set breadcrumbs for changelog and feedback pages
* feat(test): add test to assert breadcrumbs
* fix(test): remove changes
* feat(app): update breadcrumbs to use semantic nav element
* feat(test): add breadcrumb assertions to changelog and feedback pages
* fix(app): replace breadcrumb nav element with div containing data-breadcrumbs attribute
* fix ci failures
* resolved failures
* Regenerate schema.rb from migrations
* Fix test
* Remove schema dump noise
---------
Signed-off-by: Shibu M <23173570+DataEnginr@users.noreply.github.com>
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Kenrick Tandrian <60643640+KenTandrian@users.noreply.github.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
|
||
|
Sure Admin (bot)
|
3a9c8baf81 |
Warn before provider setup without encryption keys (#3007)
* Warn before provider setup without encryption keys * Cover provider encryption warning states * Warn on inline provider credential forms * Avoid duplicate provider encryption warnings * Add German passkey translations * Standardize provider encryption warnings |
||
|
|
8f9529fbe9 |
feat(goals): a reached goal lets go of the money it was holding (#3165)
* fix(goals): stop two goals from each claiming the same account in full
A GoalAccount with a NULL `allocated_amount` means "dedicate the whole
balance". Two of them on one account each claimed all of it, so the money
was counted twice:
Livret A, 6,000 precaution 6,000 vacances 6,000
progress: 100% progress: 100%
`Goal#backing_share_for` cannot catch this. Its pro-rata haircut only
scales FIXED earmarks, and an unallocated link contributes `nil.to_d` —
zero — to `others_fixed`, so the two links never see each other. The
invariant "shares never sum past the balance" held for every earmark
except the one that claims everything.
Enforce it at the door: GoalAccount now refuses a second whole-balance
link on an account another non-archived goal already claims in full, and
asks for an amount instead. The scope matches
`Goal.pooled_allocations_for` — archived goals are excluded from the
backing math, so they do not block; completed goals still hold their
money, so they do.
Rows written before this guard stay readable and editable. Autosave
revalidates every loaded goal_account on `goal.save`, so validating
untouched links would make a goal that merely holds a legacy overlap
impossible to rename. Only a new link, or one whose amount is being
cleared onto a contested account, is checked.
The goal fixtures encoded exactly the forbidden state — three goals
claiming `depository` in full — so tests that built a fourth whole
claim now use accounts of their own. `build_goal` mirrors the old
balance, leaving every KPI figure unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4
* fix(goals): keep a restored goal from re-claiming an account in full
Addresses review feedback on #3160, raised independently on #3165, #3166 and
#3167 — one bug seen four times, because those branches stack.
Archiving a goal releases the accounts it claimed in full, so another goal can
legitimately claim one while it is away. Restoring it then put two
whole-account links back on the same account and reopened the double-counting
`whole_account_link_must_be_exclusive` closes: that check only fires when a
link is written, and a state change writes none.
A validation rather than an AASM guard. `may_fire_event?` stays true, the save
fails, and GoalsController#perform_transition! already surfaces
`errors.full_messages` — the user reads which goal holds the account instead of
a generic "can't do that in this state".
The conflict lookup now lives in one place, `Goal#whole_account_conflicts_on`,
read both by the door (writing a link) and by the restore, so the two cannot
drift into disagreeing about which goals still hold their money. The
`new_record? || will_save_change_to_allocated_amount?` bound stays on the
validation rather than moving into the shared lookup: it exists so a goal
merely holding a legacy overlap can still be renamed.
`Goal::RELEASED_STATES` replaces the repeated "archived" literal. The old
comment already said this scope had to move with the pool's; the constant makes
that true rather than hoped for, and the restore guard will follow the day the
set grows.
Restores from a released state are guarded; `resume` from `paused` is not. A
paused goal never let go of its accounts, so nothing can legitimately have
claimed one meanwhile, and blocking it would strand a user on a goal they
merely shelved.
Two things this surfaced in the test data:
- The fixtures had three goals each claiming `depository` in full — the exact
state the rule forbids. `test "AASM transitions"` failed on it, a true
positive. Two of them now take a fixed 1,000 slice.
- `current_balance sums linked account balances` asserted the gross balance,
which only held because of that overlap. A whole-account link takes what is
left after other goals' fixed earmarks; the test now says so, and computes it
from the data rather than a constant.
Each guard was confirmed load-bearing by removing it and watching its test
fail. bin/rails test: 6936 runs, 27911 assertions, 0 failures. RuboCop,
erb_lint and Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): treat moving a whole-account link as the fresh claim it is
Addresses review feedback on #3160.
The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.
The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.
bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop a link that is changing goals from conflicting with itself
Addresses review feedback on #3160, on the widening that landed in
|
||
|
buzzromainandClaude Opus 5
|
7a86ee47c5 |
fix(goals): stop two goals from each claiming the same account in full (#3160)
* fix(goals): stop two goals from each claiming the same account in full
A GoalAccount with a NULL `allocated_amount` means "dedicate the whole
balance". Two of them on one account each claimed all of it, so the money
was counted twice:
Livret A, 6,000 precaution 6,000 vacances 6,000
progress: 100% progress: 100%
`Goal#backing_share_for` cannot catch this. Its pro-rata haircut only
scales FIXED earmarks, and an unallocated link contributes `nil.to_d` —
zero — to `others_fixed`, so the two links never see each other. The
invariant "shares never sum past the balance" held for every earmark
except the one that claims everything.
Enforce it at the door: GoalAccount now refuses a second whole-balance
link on an account another non-archived goal already claims in full, and
asks for an amount instead. The scope matches
`Goal.pooled_allocations_for` — archived goals are excluded from the
backing math, so they do not block; completed goals still hold their
money, so they do.
Rows written before this guard stay readable and editable. Autosave
revalidates every loaded goal_account on `goal.save`, so validating
untouched links would make a goal that merely holds a legacy overlap
impossible to rename. Only a new link, or one whose amount is being
cleared onto a contested account, is checked.
The goal fixtures encoded exactly the forbidden state — three goals
claiming `depository` in full — so tests that built a fourth whole
claim now use accounts of their own. `build_goal` mirrors the old
balance, leaving every KPI figure unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4
* fix(goals): keep a restored goal from re-claiming an account in full
Addresses review feedback on #3160, raised independently on #3165, #3166 and
#3167 — one bug seen four times, because those branches stack.
Archiving a goal releases the accounts it claimed in full, so another goal can
legitimately claim one while it is away. Restoring it then put two
whole-account links back on the same account and reopened the double-counting
`whole_account_link_must_be_exclusive` closes: that check only fires when a
link is written, and a state change writes none.
A validation rather than an AASM guard. `may_fire_event?` stays true, the save
fails, and GoalsController#perform_transition! already surfaces
`errors.full_messages` — the user reads which goal holds the account instead of
a generic "can't do that in this state".
The conflict lookup now lives in one place, `Goal#whole_account_conflicts_on`,
read both by the door (writing a link) and by the restore, so the two cannot
drift into disagreeing about which goals still hold their money. The
`new_record? || will_save_change_to_allocated_amount?` bound stays on the
validation rather than moving into the shared lookup: it exists so a goal
merely holding a legacy overlap can still be renamed.
`Goal::RELEASED_STATES` replaces the repeated "archived" literal. The old
comment already said this scope had to move with the pool's; the constant makes
that true rather than hoped for, and the restore guard will follow the day the
set grows.
Restores from a released state are guarded; `resume` from `paused` is not. A
paused goal never let go of its accounts, so nothing can legitimately have
claimed one meanwhile, and blocking it would strand a user on a goal they
merely shelved.
Two things this surfaced in the test data:
- The fixtures had three goals each claiming `depository` in full — the exact
state the rule forbids. `test "AASM transitions"` failed on it, a true
positive. Two of them now take a fixed 1,000 slice.
- `current_balance sums linked account balances` asserted the gross balance,
which only held because of that overlap. A whole-account link takes what is
left after other goals' fixed earmarks; the test now says so, and computes it
from the data rather than a constant.
Each guard was confirmed load-bearing by removing it and watching its test
fail. bin/rails test: 6936 runs, 27911 assertions, 0 failures. RuboCop,
erb_lint and Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): treat moving a whole-account link as the fresh claim it is
Addresses review feedback on #3160.
The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.
The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.
bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop a link that is changing goals from conflicting with itself
Addresses review feedback on #3160, on the widening that landed in
|
||
|
buzzromainandClaude Opus 5
|
ed28fab807 |
fix(trades): let the header call a trade what the importer recorded (#3150)
The header derived its wording from the amount's sign and ignored investment_activity_label, except for Dividend and Interest. So every other labelled trade was announced as a buy or a sell: a Questrade journal, which records "Transfer", opened as a purchase it never was. The label now wins when this view has wording for it, and the sign remains the fallback for a trade with no label, or one carrying a label this view does not name. `I18n.exists?` guards that second case, so an unknown label degrades to buy/sell rather than rendering a missing-translation string. Wording added for the rest of Trade::ACTIVITY_LABELS. English only: fallbacks are enabled application-wide, so the other seventeen locales inherit it rather than raising. Not fixed by this, in case the title suggests otherwise: Kraken records its Contribution and Withdrawal labels on Transactions, not Trades, so it never reaches this view — its labels already display through the quick-edit badge. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
3d6a8d8b6e |
feat(budgets): move money between envelopes in one gesture (#3164)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
buzzromainandClaude Opus 5
|
1fddb4d97c |
feat(budgets): carry a category's unspent budget into the next month (#3143)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
Juan José Mata
|
b438a1c8dd | Improve chat tracking and system test assertions | ||
|
colinedwardwood
|
b1df16b0a7 |
Allow API transaction create to opt into sync protection (user_modified) (#3162)
* Allow API transaction create to opt into sync protection (user_modified) The transactions API has no way to mark a newly-created transaction user_modified, which is the only thing that protects an entry from a later provider sync (Plaid/SimpleFin/etc.) silently overwriting its category or name - Account::ProviderImportAdapter#import_transaction claims any entry matching on date/amount/currency with no external_id yet, then enriches unlocked fields from the sync payload. This matters for any API client that owns writes into an account also linked to a bank-sync provider: without a way to protect its own entries, the client's data can be silently overwritten the first time the linked provider happens to sync a matching transaction. Adds an optional `user_modified` param to POST /api/v1/transactions, reusing the existing Entry#mark_user_modified! (added for #1977, so far only wired into the merchant merge/convert/unlink flows) rather than mass-assigning the column directly. Exposes user_modified in the transaction JSON response, matching how external_id/source already are. Scoped to create only, matching the concrete need; happy to extend to update in a follow-up if that's wanted too. * fix: mark entry user_modified before enqueueing account sync sync_account_later enqueued the background sync job before mark_user_modified! ran, leaving a window where a fast-running job could read and overwrite the entry before the protection flag was set. Move the mark_user_modified! call ahead of the sync enqueue so the flag is always in place first. |
||
|
|
3a50846cc7 |
feat(mcp): support Streamable HTTP transport (protocol 2025-06-18) (#2631)
* feat(mcp): support Streamable HTTP transport (protocol 2025-06-18) Bumps the MCP protocol version from 2025-03-26 to 2025-06-18 to support clients using the Streamable HTTP transport, notably Bifrost v1.6.3+. Changes: - Add after_action hook to set Mcp-Protocol-Version header on all responses - Generate and return a sessionId in the initialize response - Set Mcp-Session-Id header on subsequent responses Without these headers, Bifrost's MCP client retries the initialize handshake 5 times and ultimately fails with a context deadline exceeded error, even though every JSON-RPC message is processed correctly. * test: update mcp protocol expectations * fix(mcp): negotiate protocol versions * fix(mcp): align transport error responses --------- Co-authored-by: secretsound <secretsound@users.noreply.github.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Juan José MataandClaude Opus 5
|
d6462f5fc9 |
feat(snaptrade): add device-flow OAuth alongside the browser redirect (#3126)
* feat(snaptrade): add device-flow OAuth alongside the browser redirect SnapTrade could only be connected through the authorization-code + PKCE flow, which needs a confidential OAuth client: SNAPTRADE_OAUTH_CLIENT_SECRET and a redirect URI registered on the OAuth app. A deployment that cannot register one had no path at all. Add the device grant (RFC 8628) as a second way to obtain the same token, so people can pick the flow that suits their deployment. Both grants end at SnaptradeItem#apply_oauth_tokens!, so a device-authorized item is indistinguishable from a redirect-authorized one from there on -- same Bearer data calls, refresh, revocation and sync. Nothing about existing authorized items changes: no schema change, no migration, and the PKCE path is untouched. - Provider::Snaptrade gains start_device_authorization and poll_device_token, with endpoints read from SnapTrade's OAuth metadata document (cached). - oauth_configured? now means "some flow is available" (public client id), which is what gates syncing and the provider panel; the new authorization_code_configured? gates the redirect flow specifically. - Token and revocation requests authenticate as a public client when no secret is configured -- client_id in the body instead of HTTP Basic. Without this a device-authorized item would authorize fine and then fail at its first token rotation. - The settings panel offers both when both are available; every other entry point picks one through SnaptradeItemsHelper#snaptrade_authorize_path. - The device page carries a failed attempt's code back into the form, so "not confirmed yet" is a retry rather than a restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): keep the provider panel's setup-step keys and cover both flows Two test_unit failures from the panel change. The setup steps were reordered and their keys renamed, which orphaned the translations twelve locales already had for them and broke the test asserting `oauth_setup_step_3`. The rename bought nothing: reword the steps in place instead, leaving the callback URL on step 2 where the interpolation lives. The panel tests stubbed `oauth_configured?`, which no longer decides which buttons render -- that is now `authorization_code_configured?`. Stub both, so the "configured" cases test the deployment they name, and add the device-only case that was previously unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): address device-flow review findings Two real bugs from the bot reviews, plus consistency work. The completion form posts into the `drawer` frame so errors re-render in place, but a successful redirect was then followed as a frame navigation. Both destinations carry the layout's empty `drawer` frame, so Turbo swapped that in and merely closed the dialog: the notice was lost and `return_to=setup_accounts` never advanced. Success now breaks out with a redirect stream action, the same mechanism holdings and categorizes already use, while errors keep rendering in the drawer. RFC 8628 §3.1 requires a confidential client to authenticate its device authorization request, and the panel offers the device code on deployments that configured a secret. That request now carries the same client authentication as the token request. Token endpoint resolution is now shared by all three grants, since whatever issued a token has to be what refreshes it. It reads the discovery document only when already cached and never fetches it, so the browser flow keeps working off the constant it has always used -- no new network call on refresh and no new way for an existing authorized item to fail. Also: the drawer no longer asks the provider whether it is configured, the controller tells it; and the test helpers restore the previous OAuth config rather than clearing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): reject a device authorization response that cannot drive the flow A 2xx missing device_code, user_code or a verification URI was passed straight to the drawer, which then rendered a blank code and a link to nowhere -- a dead end the user could only abandon. Every one of those fields is load-bearing, and a response without them is partial or schema-changed, so fail with a message instead. Same reasoning as the results-array check in get_positions. verification_uri_complete substitutes for verification_uri when present, since the drawer prefers it for the link anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): filter device-flow codes from request logs complete_oauth_device_flow receives the device code as a request parameter, and none of the existing filter_parameters patterns is a substring of "device_code" -- ParameterFilter matches on substrings, and "token", "_key", "secret", "code_verifier" and "code_challenge" all miss it. So Rails' default "Processing by ... Parameters: {...}" line was writing it in plaintext. That matters more here than ordinary log hygiene: the device code is the only capability check on redemption. Unlike the redirect flow's state, nothing binds a device code to the family that requested it, so anyone who can read the logs could redeem another family's in-flight authorization into their own item and pick up a token for that family's brokerage data. Adds :device_code, :user_code and :verification_uri_complete (which embeds the user code) to the filter list, with a regression test in the style of the existing Sophtron credential-filtering test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): bind a pending device authorization to its session The device code was posted back from the drawer as a form field, so the request body was the only thing deciding which item a pending authorization redeemed into. Nothing tied a code to the family that asked for it -- the guarantee `state` gives the redirect flow -- so a code recovered from anywhere could be redeemed into an item belonging to someone else, handing them a token for the victim's brokerage data. Hold the pending authorization in the session instead, where oauth_callback already keeps its code_verifier and state: - start_oauth_device_flow records the code, what the page displays, the family, the item and the return_to context under :snaptrade_device_flow. - complete_oauth_device_flow reads the code from there and refuses unless the flow was started by this session for this family and this item. A device_code parameter is no longer read at all, so there is no longer a way to inject one. - return_to and accountable_type come from the session too, so completion needs nothing from the form to find its way back. The code now never reaches the browser, which also makes the previous commit's log filtering a second line of defence rather than the only one. A failed attempt keeps the code only while it is still redeemable: expired_token and access_denied clear it so the page offers a fresh start, while a transient failure leaves it in place to retry. expires_in and interval are no longer carried anywhere, since nothing ever read them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): use one token endpoint for every grant poll_device_token resolved the token endpoint from the cached discovery document while exchange_code and refresh_tokens used TOKEN_URL, so which URL a device-issued token was refreshed at depended on whether the 12h metadata cache was still warm. If the discovered endpoint ever differed from the constant, a device-authorized item would work until the cache lapsed and then fail its first rotation -- and fail invisibly, since a refresh failure marks the connection requires_update. Resolve it by removing the choice rather than by making refresh depend on discovery. RFC 8628 §3.4 redeems a device code at the authorization server's token endpoint, the same one the authorization code grant uses: there is one token endpoint, not one per grant, and nothing to keep in sync between issuing a token and refreshing it. TOKEN_URL is also the endpoint the browser flow has been using in production, so it is the one with evidence behind it. Discovery is still consulted, but only for device_authorization_endpoint, which has no hardcoded equivalent. This also keeps refresh free of any network dependency it did not already have: reintroducing discovery there would have put a fetch, with retries and backoff, in front of every token rotation on items that never needed one. Also restore the previous OAuth configuration in the missing-client-id test instead of leaving the client id nil, which made it order-dependent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
0aa43de10a |
Add experimental Swift-native Sure Insights app (#3134)
* Add Swift-native Sure app * Fix push subscriptions schema for CI * Address native app review feedback * Address remaining native app review feedback * Use Flutter app logo for native icon * Honor insight notification preferences and locale --------- Co-authored-by: Juan Jose Mata <2v8shcb6pz@privaterelay.appleid.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
|
3e79de5b64 |
perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries (#3058)
* perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries The account sidebar renders on every page (mobile + desktop, 3 tabs each) and calls Family#balance_sheet multiple times per render; neither it nor Family#investment_statement/InvestmentStatement#current_holdings were memoized, so each call rebuilt the underlying query from scratch. Memoize both per-user (family sharing means different users must not share a cached BalanceSheet/InvestmentStatement). Also cache TransactionsController#index's uncategorized_count and projected_recurring lookups, which run unconditionally on every request regardless of whether the underlying data changed, using the same entries_cache_version-keyed pattern already used elsewhere in the codebase (e.g. Transaction::Search#totals). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address PR #3058 review feedback on cache invalidation and query reuse - Invalidate transactions-index caches when the current user's AccountShare access changes, not just on entries/recurring updates (CodeRabbit/Codex flagged revoked users could see stale data for up to a day). - Use full-precision timestamps instead of to_i in the cache keys so same-second updates aren't missed. - Reuse the already-memoized investment_account_ids in InvestmentStatement#current_holdings instead of an extra any? query. - Assert the rendered response instead of a controller instance variable in the uncategorized_count test, per CodeRabbit nitpick. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: assert rendered response, not implementation details, in PR #3058 tests Two CodeRabbit nitpicks from the second review round: the uncategorized-count cache-reuse assertion matched a scope name that never appears in generated SQL (making it vacuous), and the recurring-cache revocation test read the controller's private @projected_recurring ivar instead of the rendered page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bust transactions-index caches on entry/recurring deletion and account status change jjmata's PR review flagged two invalidation gaps: hard-deleting an uncategorized entry or recurring transaction left the previous max updated_at unchanged (cache never busted), and toggling an account's active status doesn't touch entries/AccountShare at all. Fold in counts (like account_share_version already did) and a new Family#accounts_status_version, and move the version helpers onto Family/Current per the "fat models, skinny controllers" nit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include merchant version in projected_recurring cache key Editing or deleting a FamilyMerchant doesn't touch recurring_transactions, so the cached projected-recurring list (rendered with merchant name/logo, expires_in: 1.day) could show stale merchant data for up to a day. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(transactions): version projected-recurring cache by referenced merchants, not just FamilyMerchant recurring_transactions.merchant_id can point at a shared ProviderMerchant (recurring detection copies transaction.merchant_id), not just a family-owned FamilyMerchant. merchants_version only tracked Family#merchants (FamilyMerchant), so a ProviderMerchant update (e.g. ProviderMerchant::Enhancer setting name/logo) left the cached projected recurring list stale for up to a day. Replace Family#merchants_version with #recurring_transaction_merchants_version, scoped to the merchant records actually referenced by the family's recurring transactions (both FamilyMerchant and ProviderMerchant), and bump the cache key version. Also fix a flaky test assertion that matched all recurring_transactions-table queries instead of the actual projection query, since computing the cache key itself still runs small COUNT/MAX queries against that table on a cache hit. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |
||
|
Juan José Mata
|
fd6f4ff078 |
Add live AI checks to system health (#3155)
* Add live AI checks to system health Give super admins a dedicated AI status view with bounded liveness probes for LLMs, vector stores, pgvector, and embedding endpoints. Record sanitized failures in both the system debug log and Rails logger, and document the recommended local configuration.\n\nCloses #3145 * Fix AI health CI checks * Address AI health review feedback * Correct Ollama model preload guidance * Distinguish OpenAI-compatible providers * Make Ollama startup readiness explicit * Recognize Cloudflare AI endpoints |
||
|
buzzromainandClaude Opus 5
|
0d0003b032 |
fix(onchain-wallets): show linked wallets on the accounts page, and drop the tracking row on unlink (#3136)
Two gaps in #3081 after it merged, both raised there by @jjmata's review of #2191 — credit to that PR for identifying them. **Linked wallets were invisible on /accounts.** The feature creates real Sure accounts and they count towards net worth, but the accounts page never showed them: `Account.manual` excludes anything carrying a provider link, and unlike the twenty other providers there was no on-chain section to claim them. A family whose only connection was a wallet saw the empty state on the one page meant to list their accounts. The controller now loads the items, the view renders them, and the empty-state condition counts them. The card is rendered by name rather than as a collection, because the model's default partial slot is already the provider settings row; renaming it would be the tidier convention but costs eight locale files of churn for a follow-up fix. **A generic unlink left the tracking row behind.** The dedicated disconnect flow is careful, but `AccountsController#unlink` destroys the AccountProvider directly, and an OnchainWalletAccount holds that link rather than being held by it. Orphaned, it stops syncing — the syncer only reads linked rows — while its partial unique index still holds the (item, chain, address, asset) slot, so linking that same asset again would collide with a row nothing displays. It is destroyed with its link now, following the CoinStats precedent in the same model, with a guard against the recursion that precedent lacks. The regression test is the one asked for explicitly: the Sure account and its holdings survive as manual while the tracking row and the provider link go. **The card only renders accounts the viewer may see.** Found in review of this branch. An item is surfaced as soon as ONE of its accounts is accessible, so rendering them all showed a member given access to one wallet account the names and balances of the others. Reproduced before fixing, with two real addresses under one item and a member shared into only one: the unshared account's name appeared on /accounts. Non-admins now get the accessible subset and the address count derives from it; admins keep the whole item, which is the rule visible_provider_items already applies. That pattern is not specific to this card — six existing providers pass `item.accounts` unfiltered to the same partial, which filters nothing. In the same reproduction the unshared name appeared twice, once from a CoinStats card over the same accounts. Raised separately for the maintainers; only this card is changed here. Both figures are computed on the item off the preloaded associations and prepared per card by the controller, the way `_coinstats_sync_stats_map` already is: a first attempt did it in the template with a `linked` scope, which opened a fresh relation and cost a query per row. Measured on /accounts with 1 then 5 wallet items: 52->79 queries became 50->69, so 6.75 per extra item became 4.75. Verified beyond the suite: a real Bitcoin address linked, then both flows driven over real HTTP — `GET /accounts` renders the card, and `DELETE /accounts/:id/unlink` leaves the account behind reporting `manual: true`. Integration parity was checked rather than assumed: on-chain now appears everywhere CoinStats does, the nightly family sync picks it up through its own reflection over `*_items` associations, and it is already exposed to the mobile client via /api/v1/accounts and to /api/v1/provider_connections. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Josh
|
ca66346dc5 |
feat: add safe admin user removal (#3131)
* feat: add safe admin user removal * fix: address user removal review findings * fix: close remaining user removal review gaps * fix: handle deleted users during session creation * fix: fail closed when session creation fails * fix: reject token issuance for inactive users |
||
|
|
735b62d9c7 |
Track self-custody wallets natively: Bitcoin, EVM and Solana (#3081)
* feat(onchain-wallets): foundation for self-custody wallet tracking
Adds the schema, models and the normalised contract every chain will
produce, with no chain implemented yet.
The central constraint of a multi-chain integration is that `case chain`
must not spread through the importer, processor, controller and views.
So a chain adapter's only job is to turn an address into an
Onchain::Snapshot — a list of Onchain::Assets and Onchain::Movements —
and Onchain::Chains is the single source of truth for which chains exist,
how their addresses are validated, what their native asset is, and which
adapter to instantiate. Everything downstream is written once.
Two tables:
- onchain_wallet_items: the family-level connection. Keyless by
default; the only credential is an optional Etherscan key, encrypted
via `encrypts`.
- onchain_wallet_accounts: one row per asset, per address, per chain.
Uniqueness uses three partial unique indexes, one per asset kind, because
the identity of an asset depends on its kind: a native coin is identified
by its address alone while a token is identified by its contract/mint. A
single index over all columns would treat two native rows with a NULL
contract_address as distinct and let duplicates through — the model test
proves the rejection comes from the database by saving with
`validate: false`.
The schema also leaves room for extended-key (xpub) wallets later: an HD
wallet is a set of derived addresses under one item, which the current
uniqueness key already allows, so adding it needs no destructive change.
db/schema.rb is hand-edited to add only the two new tables: regenerating
it with the Rails version now in the Gemfile reorders every column in the
file, which is out of scope for this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): chain-agnostic importer, processor and syncer
The whole pipeline is written once here and never branches on chain: it
consumes Onchain::Snapshots, so a chain is only ever a registry key and an
adapter.
Importer: refreshes every tracked address and records a digest of what it
saw (quantity plus movements, no timestamps) on each row. It deliberately
never creates rows — real wallets are full of spam airdrops, so a newly
seen token becomes trackable only when the user ticks it. An asset that
disappears from a wallet goes to zero rather than going stale.
Syncer: reprocesses only the rows whose digest changed. Two consecutive
syncs of an idle wallet write nothing at all — no row updates, no
holdings, no entries, and no queued account syncs. Both the importer and
syncer tests for that were checked against the pre-fix behaviour: with the
content_hash guard removed they fail.
Processor: writes the holding, the account balance and the movements.
Movements materialise two ways. When that day's price is known, a signed
trade (positive = Buy, negative = Sell) so cost basis and the value chart
reconstruct back to acquisition. Otherwise a display-only entry with
amount 0 and excluded: true, raw movement preserved in `extra` — visible,
but not inventing a value that would distort the account's history. Prices
are matched on the exact day for trades, because valuing a two-year-old
transfer at today's price would fabricate a cost basis.
Onchain::SecurityResolver binds a "CRYPTO:<SYMBOL>" ticker straight to the
crypto price provider instead of going through provider search, where
"USDC" can come back as a EUR-quoted pair and then need FX to repair. It
reuses an existing Security for the ticker whatever its MIC, so one asset
never splits into two records. Symbol normalisation for bridged
stablecoins and the price backfill follow in the next commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): canonical asset symbols and price backfill
Two things stood between a linked wallet and a correct valuation.
Bridged and wrapped variants. The same dollar is USDC on Ethereum, USDC.e
on Arbitrum and USDbC on Base; the same ether is ETH natively and WETH
once wrapped. Left alone each variant becomes its own Security, so one
gets priced and the others sit at zero, and the same asset held on two
chains reports two different values. Onchain::AssetSymbol maps the
variants that are redeemable 1:1 onto the canonical asset, so pricing them
as that asset is exact rather than approximate.
Missing price history. A Security created at link time has none, so on the
first sync every movement would fall back to a display-only entry and the
cost basis would never reconstruct — the feature would look broken exactly
when the user first looks at it. The processor now backfills the window in
one batched provider call rather than one call per movement date, includes
today so a wallet whose movements all predate the sync window still gets a
current valuation, and treats a provider failure as a logged warning: the
holding and the quantity are still written.
All of it is a no-op when no crypto price provider is enabled, which is
the case the settings panel has to warn about before linking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Bitcoin adapter
Bitcoin has no account balances: an address owns unspent outputs, so the
balance is everything ever paid to it minus everything spent from it, and
mempool totals count — a broadcast-but-unconfirmed spend has already left
the wallet as far as its owner is concerned. Movements are the net effect
of each transaction on the address, so a self-transfer nets to zero and
produces no entry.
All three address formats are accepted (Base58 P2PKH/P2SH, bech32
segwit, bech32m taproot) with the character-set exclusions each encoding
actually has, and a malformed address is rejected before any request is
made — the test relies on WebMock failing the run if a request escapes.
Single address, not extended keys. A Bitcoin wallet is normally an HD
wallet: one xpub derives thousands of addresses and change goes to derived
ones, so tracking a single address under-reports such a wallet. Extended
keys would need BIP32 derivation (a dependency this codebase does not
want) or a descriptor-indexing backend. The limitation is stated in the
adapter, will be stated in the linking UI, and an xpub is rejected as an
address rather than silently treated as one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): EVM adapter for six networks, two backends
One adapter class serves every EVM network: a network's identity — label,
native coin, explorer URL, whether a family-supplied Etherscan key applies
— is data in the chain registry, so adding a network is an entry there
rather than a branch here.
The unit tracked is the (chain, address) couple, carried by the unique
index. A 0x address is valid on all six networks and holds different
balances on each, so detection asks each candidate network whether the
address is worth tracking there. That probe is exactly one request and
never reads paginated history: Blockscout's address summary carries the
coin balance and the token/transfer flags together, which is also why a
wallet holding only ERC-20 tokens with zero native balance is still found.
An explorer being down means "not detected here", not an error the user has
to interpret — a dead indexer must not break linking.
Two interchangeable backends behind Provider::EvmExplorer: keyless
Blockscout by default for every network, and Etherscan when the family
configured a key on a network the registry enables it for (today Ethereum
only), where a key buys nothing but a higher rate limit. Etherscan
deliberately does not implement the activity probe: its one-request answer
is the native balance, which reports "nothing here" for a token-only
wallet, so detection stays on the indexer that can answer correctly.
Zero-balance token rows are dropped — real wallets are full of spam dust —
while the native asset is always reported, even at zero, because a wallet
that spent everything still has a history worth keeping.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Solana adapter
A Solana wallet does not hold its tokens. Each SPL token sits in its own
token account, owned by the wallet but addressed separately, so balances
come from enumerating those accounts across both token programs rather
than from reading the wallet address — and because one wallet can own
several accounts for the same mint, they are summed into one position.
Emptied token accounts are left behind on chain by design and are dropped.
RPC gives mints, not metadata. Well-known mints get their real symbol;
anything else is labelled with its mint in a form that deliberately cannot
pass for a ticker, so security resolution declines it and the asset is
tracked by quantity rather than priced as some unrelated coin that happens
to share a name.
Activity is one request. Bitcoin's Base58 addresses fall inside Solana's
address shape, so the two are told apart by asking the node: it rejects a
non-32-byte key, which reads as "not here" rather than an error.
The Snapshot it returns has the same shape as Bitcoin's and the EVM
adapter's — asserted in the test — so nothing downstream changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): settings panel and linking flow
Linking is three steps: paste an address, confirm which network, choose
what to track.
The network step exists because address formats are not unique to a chain.
Candidates come from the address shape, and when there is more than one each
is asked whether the address is worth tracking there — one bounded request
each. When several answer yes, or none does, the user picks from a list that
marks which ones showed activity. Silently keeping the first match would
link the wrong network and surface later as a sync bug.
The token step imports nothing that was not ticked. Assets whose symbol a
price provider can quote are pre-checked; spam airdrops, whose "symbol" is
usually an advertisement, are listed unticked and can still be tracked by
quantity. Quantities and metadata are re-read from the chain when the
selection is applied, so a tampered selection can only change which assets
are tracked, never what they claim to hold. Previewing an address creates
no connection record, so an abandoned flow leaves nothing behind.
The panel and both modal steps carry a price-provider warning: with no
crypto market data enabled every wallet is valued at zero, which users
report as a broken sync rather than a missing setting, so it is said before
linking and links to where to fix it. The Bitcoin single-address limit and
"never enter a seed phrase" are stated in the linking UI too.
Errors are separated by kind. A rate limit or an unreachable explorer gets
its own localized message. Anything else is a bug: the user sees a generic
message and the class and message go to DebugLogEntry, never into the
response — asserted in the tests, which also check the exception text does
not appear in the body.
Adapters now translate their data source's errors into Onchain::Chains
errors, so the controller rescues two chain-agnostic types instead of
carrying a list of every explorer's error classes — which would have put
per-chain knowledge back into the controller.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): manage, review tokens, change address, disconnect
Four actions, because two would make the token choice irrevocable.
Review tokens reopens the selection screen with the address left alone —
deliberately without an address field. Without it the only way to untick a
token would be to change the address, which is a different operation
entirely. Assets the chain no longer reports stay listed and ticked, so
they can be dropped once they are gone.
Disconnect one asset is a per-row action with a button next to every asset.
A destroy route no view calls is dead code: the feature does not exist
until it has a button, so the test asserts one form per asset rather than
one per wallet.
Change address updates the existing rows instead of recreating them, so the
accounts, holdings, entries and balance history all survive — verified by
asserting a pre-existing balance is still there afterwards, and checked
against the recreate-instead-of-update behaviour, which fails it. The
content digest is cleared so the next sync reprocesses even if the new
address happens to hold the same amount, and an account still carrying its
generated name is renamed to match.
Disconnect wallet drops every asset at one address and leaves other
addresses alone.
Disconnecting never destroys an account: the provider link goes, holdings
are detached, and what the user can see stays as a manual account that
stops updating — the same contract every other provider's unlink has here.
The duplicate-address guard covers initial linking as well as address
changes. Without it, re-linking an already-tracked address would become the
unofficial way to add a token, quietly creating a second set of rows for
the same wallet; both guards are checked against that pre-fix behaviour.
Every lookup and mutation is scoped through Current.family, including the
per-row disconnect, which cannot reach a row belonging to another
connection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): hosting guide for self-custody wallet tracking
docs/hosting/onchain-wallets.md covers what the feature reads, which
endpoint serves each network and how to point it at your own instance, the
optional Etherscan key and why it is optional, the per-sync request cost and
where history is capped, and the management actions.
Two things get stated plainly because they generate the support traffic:
prices come from a separate market data setting, so with no crypto-capable
provider enabled every wallet is tracked by quantity and valued at zero; and
Bitcoin is one address at a time, which under-reports an HD wallet whose
funds are spread across derived addresses.
Every locale key the feature uses is checked to resolve, including the
pluralised ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep EVM balances on the keyless indexer when a key is set
Configuring an Etherscan key moved every read onto Etherscan, including
balances. That was wrong: Etherscan has no free endpoint that enumerates an
address's tokens, so its token balances are summed from transfer history —
which cannot see a rebasing token's current balance, and silently
under-reports any wallet whose history exceeds the page cap. A user adding a
key to fix a rate limit would have quietly traded it for wrong balances.
The two reads are now separated by what each backend can actually answer.
Balances and activity detection always go to the keyless indexer, whose
address summary answers both in one request — so adding a key buys nothing
there and cannot cost anything either. History, the paginated and
rate-limited half, is where a key helps and is the only thing it changes.
Provider::Etherscan drops its balance methods and refuses token_balances
with the reason, rather than offering an approximation that reads as a fact.
The chain-agnostic error mapping now accepts several error families per role,
since one snapshot can involve both backends.
Checked against the pre-fix behaviour: with balances routed through the
keyed backend, the new test reports 1 USDC held instead of the 7 the indexer
sees, and Etherscan raises on the balance call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): name verified SPL tokens so they can be priced
Solana RPC returns mints, not names, so every SPL token outside the
hard-coded handful showed up as "SPL:abcd…wxyz" — which security resolution
correctly declines, leaving the asset tracked by quantity and valued at zero.
For a wallet holding anything beyond USDC that was most of its value.
Names now come from Jupiter's keyless token search, asked once per snapshot
for every mint at once, cached 24 hours per mint.
Only mints the list reports as *verified* are trusted. Anyone can mint a
token calling itself USDC; naming an unverified one would hand it the real
dollar's price and value dust at thousands. Unverified and unknown mints keep
the placeholder, and the test for that asserts the spam token is not named
USDC. Misses are cached as well, because spam wallets hold many mints that
will still be unvouched-for tomorrow.
Metadata is a naming nicety, not the wallet: a list that is down or rate
limiting degrades to placeholders instead of failing the snapshot, and the
hard-coded mints stay as an offline floor. Assets and movements resolve from
the same lookup, so a token cannot be called two different things within one
snapshot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): enable crypto pricing in one click, and log zero valuations
The warning said what was wrong and where to fix it, but still made the user
navigate to another settings page and pick the right provider out of a list.
On a self-hosted instance an admin can now fix it from the warning itself:
the crypto provider is appended to the enabled securities providers, leaving
the others alone — enabling crypto prices must not turn off whatever prices
the user's equities, and the test fails if it does. On a managed instance the
providers are the operator's setting, so the button is not offered and the
action refuses.
The other half is diagnosis. Until now a holding valued at zero for this
reason looked exactly like a holding whose price simply had not been fetched
yet: nothing recorded it, so support had to infer it from a screenshot. The
processor now records it via DebugLogEntry — but only for this cause, since a
price merely missing for today is ordinary and already covered by the
backfill.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): upgrade display-only movements once their price is known
A wallet linked before market data covered its history got the worst of both
worlds: its transfers landed as zero-amount excluded entries, and nothing ever
brought them back. The syncer only reprocesses assets whose on-chain state
changed, and a two-year-old transfer never changes — so the cost basis stayed
broken for exactly the wallets that were linked earliest.
perform_post_sync now runs a repair over every linked asset, not just the ones
that moved: what changed is the price history, which no chain read can report.
It reads prices from the database only, makes no network call, and does
nothing when there is nothing to upgrade.
An upgraded entry keeps its external_id, so it is the same transfer rather
than a duplicate. The entry is destroyed and rewritten because an Entry cannot
change entryable type in place and import_trade refuses an id already held by
a Transaction. Entries from other providers are never touched — matched by
source and by this asset's own id prefix.
Checked against the pre-fix behaviour: with perform_post_sync empty again, the
syncer test fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): make history truncation visible and its depth configurable
History was capped silently. A wallet with more transfers than one sync reads
looked exactly like a wallet whose history was fully imported — the only trace
was a Rails log line on Bitcoin, and nothing at all on the EVM and Solana
paths. A user reconciling their cost basis had no way to tell the difference
between "this is all there was" and "we stopped reading".
Each source now reports whether it stopped on the budget or on the end of the
history. That travels on the Snapshot, so it stays chain-agnostic; the flag is
recorded on the affected rows, the manage screen says the history is
incomplete for that address, and the importer records it once per address per
sync — only when something changed, so an idle wallet with deep history does
not log the same line every night. The message also states what is not
affected: balances come from an address summary, never from history.
The depth itself is a hosting decision, not a property of a chain, so it moves
out of the providers into Onchain::HistoryBudget and is settable with
ONCHAIN_HISTORY_MAX_PAGES (default 10, clamped to 200). Adapters inject it, so
the providers stay unaware of the policy and the budget is testable without
touching the environment.
What this does not do is resume where it stopped. Reading older history across
syncs needs a per-wallet cursor and changes what the content digest means —
which is what guarantees an idle wallet writes nothing — so it is a change of
its own rather than a rider on this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): read Blockscout token balances the way the API accepts
Verified against the live API: /api/v2/addresses/{address}/token-balances
rejects a `type` filter with HTTP 422 — the filter exists on token-transfers,
not here. Every EVM snapshot was therefore failing in production and
surfacing as "the explorer could not be reached", while the tests passed
because the stub encoded the same wrong assumption. No unit test can catch a
mistaken API contract; only asking the API can.
Without the filter the endpoint answers with every token standard the address
holds — ERC-20, ERC-721, ERC-1155, ERC-404 — so ERC-20 is now selected
client-side. An NFT row carries value "1" and no decimals, so it would have
been imported as a fungible balance of one token, priced by whatever its
symbol happened to resemble.
The tests now stub the endpoint the way it really answers and assert we ask
without a query, so re-adding the filter fails the run. Each token's
market-cap signal is carried through for the next commit, which has to decide
what to do about an address holding thousands of tokens.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound how many tokens one address can surface
Probing the live APIs turned up the other thing the stubs were hiding: real
addresses are airdrop dumping grounds. A well-known Ethereum address returns
7,924 ERC-20 balances (3.2 MB), and a comparable Solana one 2,801 token
accounts. Nothing bounded either. That meant a review screen with thousands of
rows nobody can use, and on Solana — where names are looked up in batches of
50 — around 56 extra requests per sync just to label an airdrop dump.
One read now surfaces at most 200 tokens per address
(ONCHAIN_MAX_TOKENS_PER_ADDRESS, clamped to 5,000). The native coin is never
affected, and anything already tracked keeps syncing regardless of the cap.
What survives the cap has to be both sensible and stable. On EVM the tokens
are ranked by the market cap the indexer already reports, so real assets stay
and airdrops fall off the end. Solana RPC gives no such signal, so the order
is the mint address: arbitrary, but identical between two reads of an unchanged
address — an unstable order would reshuffle the wallet, change the content
digest, and rewrite holdings every night. There is a test for that on both
chains. On Solana the cap is applied before metadata lookup, so it bounds the
requests as well as the rows.
The cap is not silent: it is recorded on the affected rows, stated in the token
review screen and in Manage wallets, and reported alongside history truncation
in the debug log with the limit that applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): walk the linking and management flow in a browser
The linking flow stacks three Turbo frame navigations — the provider drawer,
the linking modal on top of it, then the token review in that same modal —
and no controller test exercises any of that. Driving it in a real browser
found two things worth keeping:
The panel is reached two different ways. With nothing linked yet it is a card
under "Available" that opens the drawer; once a wallet exists the provider
moves to "Your connections", where the panel renders inline inside a
disclosure and there is no link to click at all. Only the first path had ever
been exercised.
Both paths now are, along with the parts of the flow that only exist in a
browser: assets arriving pre-ticked or not according to whether they can be
priced, review tokens reopening the selection with no address field present,
and disconnecting one asset leaving its account behind. One test per flow —
the branching cases stay in the controller test, where they cost a fraction
of the time.
Verified visually at each step: the warning banner, the Bitcoin
single-address note, the three-asset review with the spam token unticked, the
four management actions, and the accounts landing under Crypto with the wallet
subtype.
Full system suite green the documented way (DISABLE_PARALLELIZATION=true):
94 runs, 385 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): report a timed-out data source as unreachable
Reading a real Solana wallet turned up the hole: the public RPC timed out,
Net::ReadTimeout escaped every layer untranslated, and the user was told
something had gone wrong with Sure — the generic message reserved for our own
bugs — with a DebugLogEntry filed as if it were one. A public endpoint being
slow is the most ordinary failure this feature has.
All five clients now translate transport failures into their own ApiError:
timeouts, refused or reset connections, unreachable hosts, TLS errors, and a
response that is not JSON. The adapters already map a provider's own errors
onto Onchain::Chains::UnreachableError, so a timeout now reads as "the public
explorer could not be reached", the message that tells the user to retry,
while genuine bugs keep the generic one. During chain detection it goes back
to meaning "not detected here", so a slow explorer still cannot break linking.
The translated message carries only the error class, never the original
message, because a transport error's message contains the full URL and these
get logged.
Checked against the pre-fix behaviour on Bitcoin: with the translation
removed, the timeout test fails with the raw exception instead of the chain
error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): name on-chain trades after the asset, not "shares"
Running a real Bitcoin address through the app showed every imported transfer
as "Buy 0.000003 shares of CRYPTO:BTC". That name comes from the shared trade
helper, which is written for equities; a wallet does not hold shares, and the
internal ticker is not what the user calls the coin. Trades are now named
"Buy 0.000003 BTC", through i18n like the display-only entries already were.
Also drops the sync status_text calls. Sync has no such attribute in this
schema, so every one of them was a guarded no-op and the four locale strings
they referenced could never render — dead weight copied from another
provider's syncer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep balances when a source refuses history
Reading real wallets on the free Solana endpoint exposed two problems, one of
which I introduced.
The budget. Making history depth configurable scaled Solana's transaction cap
from 25 to 250, and on Solana one transaction is one RPC call rather than one
page — so the same nominal depth became an order of magnitude more expensive
and a sync went from seconds to minutes. The per-transaction budget is now its
own constant, scaled proportionally off the page knob so one setting still
moves both, with a test that pins it far below the paginated row count.
The bigger one: a failure while reading history threw away the balances too.
A balance is one bounded request and is what a wallet fundamentally is; history
is paginated, far more expensive, and the first thing a throttled endpoint
refuses. On the free Solana endpoint, which routinely throttles getTransaction,
that meant a wallet showed nothing at all rather than showing what it holds —
permanently, not transiently.
History is now best effort: when the data source refuses it, the balances are
still recorded and the history is marked incomplete, which the manage screen
already surfaces. Anything that is not the data source failing still raises, so
a bug here cannot be swallowed. Verified live: the same Solana wallet that
failed entirely now reports 52.06 SOL and its SPL tokens with the history
flagged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): pre-tick what is worth something, not what parses as a ticker
Linking a real airdropped address showed 200 of its 201 assets arriving
pre-ticked, one click away from 200 accounts — the opposite of what reviewing
tokens before import is for. The pre-tick rule was "the symbol looks like a
ticker", and airdrops use perfectly plausible short symbols (0XBTC, 4CHAN), so
it selected nearly everything.
Assets now carry whether the data source treats them as notable, and only those
are pre-ticked. Two attempts at that signal, decided by measuring the real
address rather than guessing:
- Blockscout's `reputation` is "ok" for all 6,669 tokens it holds. Useless.
- Market-cap presence looks strong on the full list (365 of 6,669) but is
useless after the cap, which already ranks by market cap — hence every
surfaced token having one.
- Holding value discriminates: of the 365 priced tokens, 112 are worth more
than a dollar.
So on EVM networks a token is notable when the indexer can price it and the
holding is worth more than a dollar; on Solana when the verified token list
vouches for the mint; the native coin always. Pre-ticked count on that address
drops from 200 to 72, and everything else stays one click away.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): warn when nothing can convert USD prices into the family currency
A family whose currency is not USD needs two settings, not one, and only the
first was covered. The single provider that prices bare crypto symbols quotes in
USD, so valuing a wallet in EUR needs an exchange rate on top — and Sure's
default exchange rate provider requires an API key, so a self-hosted install
without one has no FX at all.
Tested against a real address in a EUR family: every wallet came out at zero,
with 275 transfers recorded as unpriced, and nothing anywhere said why. That is
the same support ticket the crypto-provider banner exists to prevent, arriving
through the other door.
Onchain::Pricing answers "can an on-chain asset be valued in this currency, and
if not, why" with the two reasons separately. The linking UI states whichever
applies — naming the currency for the FX one, and pointing at Frankfurter, which
needs no key — and a USD family never sees that warning because it needs no
conversion. The processor records the reasons when a holding lands at zero, so
support sees "exchange_rate" rather than guessing.
Verified live afterwards: with EXCHANGE_RATE_PROVIDER=frankfurter, the same EUR
family values the wallet at 416,198,342.25 EUR at 0.86363 USD/EUR, with all 275
transfers priced in EUR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let a movement become a trade after being display-only
Syncing a real wallet twice — once before its prices were reachable, once after
— raised ArgumentError from the shared importer: an Entry cannot change
entryable type in place, and the display-only Transaction already held the
external_id the trade needed.
That is the ordinary case, not an edge one. A wallet linked before market data
covers its history gets display-only entries on the first sync, and the first
sync that can price them dies. Worse, it dies inside perform_sync, so the repair
pass that exists precisely to upgrade those entries — and which runs in
perform_post_sync, afterwards — was never reached. The account stayed stuck.
Both paths now go through one writer that discards a stale display-only entry
before the trade takes over its identity, so the entry keeps being the same
transfer rather than becoming a duplicate. Checked against the pre-fix
behaviour: without the discard, the new test raises the original ArgumentError.
Found by running a EUR family through two syncs on real data, which is the only
way the two price states occur in order.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): write a trade and drop its display-only entry atomically
Replacing a display-only entry with the trade it became is one change, but it
was two writes outside a transaction. A failure between them left the account
with neither: the transfer disappeared until some later sync happened to rewrite
it, which for an idle wallet could be never.
The repair pass already wrapped this; the sync path did not, and that is the one
that runs first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): tell apart two transfers of one token in one transaction
A token transfer was identified by its transaction hash and contract, which are
not unique together: a swap router or a batch payout routinely emits several
transfers of the same token involving the same address within one transaction.
The second overwrote the first, so a transfer disappeared from the account
without a trace.
Both EVM backends report the log index — the field that makes an event unique
inside a transaction — and neither was using it. It is now part of the
identifier, with the contract kept only as a fallback for an instance that does
not report one.
Reported by review on #3081; confirmed against the live Blockscout payload,
which carries log_index. The test fails against the previous identifier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): canonicalise an address per chain before anything keys off it
Addresses were only stripped, never canonicalised, and what counts as the same
address differs by chain — so the duplicate guard could be walked straight past
and one Bitcoin case was worse than a duplicate.
On EVM, hex is hex: 0xABC… and 0xabc… are one wallet, but they linked as two,
each with its own accounts and holdings for the same balance.
On Bitcoin, bech32 is case-insensitive and canonically lowercase, and the API
reports outputs that way. An uppercase bech32 address passed validation, gave a
correct balance from the address summary, and matched no output at all — so the
wallet silently had zero movements and no cost basis, with nothing anywhere
saying why.
Canonicalisation is now the adapter's answer, since only the chain knows whether
case carries identity: EVM and bech32 fold, Base58 and Solana are left exactly
as given. The controller applies it as soon as the chain is known and before the
duplicate check, and on an address change too.
Reported by review on #3081. Both tests fail without the folding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): stop folding the case of Solana mints
Contract identifiers were downcased everywhere on the assumption that a contract
address is hex. That holds for ERC-20 but not for an SPL mint, which is a Base58
public key where case is part of the value: the stored mint was an unusable copy
of the real one, and two distinct mints could collide once folded into the same
string — one wallet's balance landing on the other's row.
Whether case carries identity is a property of the token kind, so it is now
answered in one place and applied consistently: by the asset's identity key, by
the column, and by the two comparisons in Onchain::Snapshot. A Movement no longer
folds anything on its own, because a movement does not know its asset's kind.
That also removes the duplication review flagged: the asset key was derived in
three places and only one of them folded, so the review screen and the linker
could disagree about what identifies an asset. There is one definition now,
OnchainWalletAccount#asset_key.
Reported by review on #3081. The test fails with the unconditional fold.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): isolate a failing address, and let a late symbol land
Two problems in the importer, both reported by review on #3081.
One unreachable address took the whole connection down with it: the loop over a
family's addresses had no rescue, so a Bitcoin explorer being throttled left an
untouched Ethereum wallet unsynced as well. Each address is now recorded and
skipped on its own — a row we failed to read keeps its previous quantity rather
than being zeroed, since we did not learn that it holds nothing — and only a
connection whose every address failed is reported as a failed sync rather than a
quiet success.
The content digest covered quantity and movements but not the metadata written
alongside them, so a Solana mint that later gained a real symbol from the token
list produced the same digest as before: the row was never rewritten and its
placeholder label became permanent. That silently undid the token-naming work.
The digest now covers everything the update writes.
Both tests fail against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): use DS::Button for the wallet actions
Five hand-built button_to controls carried their own utility-class strings for
what DS::Button already does — sync, disconnect a connection, stop tracking one
asset, disconnect a wallet, enable crypto prices. They drifted from the design
system on size, hover and destructive treatment, and the confirm prompts were
wired by hand.
The browser test walks the two that matter, so the behaviour is unchanged; this
is the styling and the confirm handling moving to the component that owns them.
Reported by review on #3081, along with the raw `bg-amber-600` on the provider
badge — left as it is, deliberately: all 23 other providers set a raw palette
class for their badge, there is no functional token for a brand colour, and
changing one entry would make it the only different one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): four smaller findings from the review on #3081
**A failed read no longer leaves an empty connection.** `link_wallet` created the
family's connection before fetching the address, so an explorer being down left a
connection with no wallets showing in the panel as connected. Reading needs no
saved connection — which is exactly why previewing uses an unsaved one — so it is
now created only once the read succeeds.
**Assets that could not be tracked are named instead of blamed on the user.**
`success?` is `created.positive?`, and the only failure message was "pick at least
one asset to track" — so a user who ticked three assets and hit three failed
creates was told to tick something, sending them back to tick the same three.
The linker already collected which assets failed; the message now says so, and a
partial failure is reported alongside what did get tracked instead of being
dropped silently. Same fix in the token revision action, which had the same shape.
**A malformed stored amount costs its movement, not the asset.** `BigDecimal()`
on a stored payload raises, and one unparseable row would fail the whole asset's
processing — including the repair pass. These payloads are written by this code,
so it takes a row that survived a format change, but the containment is two lines.
**No dead-end link.** The warning offered "open market data settings" to
everyone, while that page is gated to self-hosted admins — the same gate the
enable button already had. Both controls are now behind it, and the warning text
stands on its own for everyone else.
Each has a test that fails against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): reject non-finite amounts and quantities
The guard added an hour ago for malformed stored amounts was incomplete for the
very case it was written for: BigDecimal parses "NaN", "Infinity" and
"-Infinity", and none of them is zero, so all three sailed through
`parse_amount` into trade materialisation. Verified rather than assumed — the
test fails without the check with PG::NumericValueOutOfRange, so the infinity
reached the insert.
Fixed at both layers that write numbers. Movements: a non-finite amount skips its
movement, like any other unparseable one. Quantities: `normalize` treats
non-finite as unknown and writes zero, because Postgres numeric stores NaN
happily and one NaN quantity would turn every total that reads it into NaN.
Reported by review on #3081.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): derive the content digest from what actually gets written
Third time the digest missed a field it was supposed to cover: first the symbol,
so a Solana placeholder that gained a real name was never rewritten; now the
truncation flags, so a wallet whose history became complete — or started being
capped — kept showing the old completeness in the UI, since the early return
fires before `extra` is touched.
Rather than add a third field to a hand-picked list, the digest is now taken from
exactly the attribute hash that is about to be written. The two cannot drift
apart, which is what kept going wrong.
That needs two things to hold. Movements are sorted before the payload is built,
so the order a source happened to list them in is not mistaken for a change. And
the hash is canonicalised before hashing, because jsonb does not preserve key
order: `extra` written as {history, assets} comes back as {assets, history}, and
hashing that raw made an idle wallet's digest flip every other sync — the tests
caught it, and it is now covered by one asserting that reordered movements are
not a change.
Reported by review on #3081. Both halves checked against the pre-fix behaviour:
without the flags two tests fail, without the canonicalisation three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): state the pricing coverage limit, and that DeFi is unseen
The hosting guide explained how to configure prices but never said what the
crypto provider actually covers. It quotes by symbol, and a symbol is not a
token's identity — measured on a real Ethereum address, two of its ten largest
token positions were quoted and the other eight, including holdings worth
roughly $406k, $141k and $74k, showed zero with their quantities tracked
correctly.
That reads as a broken sync unless it is written down, so it is now the first
limitation in the list, with the practical rule a user needs: a zero next to a
token you know is worth something means the provider does not list it, not that
the balance is wrong. Troubleshooting gains the matching entry, separating it
from the two configuration causes that produce a zero across every wallet.
Also records that DeFi positions — staked ETH, LP tokens, lending, Solana stake
accounts — are invisible, which was missing from the list entirely. A wallet
holding most of its value in a staking protocol reports a fraction of it.
Docs only; no code touched, so the suite was not re-run — the last run on this
tree was green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): do not zero an asset the token cap never reached
A tracked asset missing from a snapshot was always read as "the wallet no
longer holds it" and set to zero. That is right for a complete read, and wrong
for a capped one: at most 200 tokens per address are surfaced, so a tracked
token can be absent simply because the read stopped before reaching it. Its
balance was then wiped — a real holding removed from the user's net worth, with
a holding of zero written and the account balance reduced to match.
It is reachable: on Solana the surfaced set is ordered by mint address, which is
arbitrary, so a genuinely held USDC position on an airdropped wallet can fall
outside the cap and be zeroed on the next sync.
Absence of evidence is not evidence of absence — the same distinction this code
already makes for an address it could not read, where the row keeps its previous
quantity. When the snapshot is capped and the asset is not in it, the row now
keeps what was last known and only its completeness flags are refreshed.
Reported by review on #3081. The test fails against the previous behaviour, and
the "an asset that disappeared is set to zero" case still passes: a complete read
that no longer lists an asset still zeroes it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): rename the local `token` variables the secret scan trips on
Pipelock failed the PR with eleven "Credential in URL (high)" findings, all on
the same shape: a local variable named `token` being assigned. `token = ...`
is what a leaked credential looks like to a scanner, and the rule is a
reasonable one to keep.
Renamed rather than excluded. Excluding paths or adding `# pipelock:ignore`
would have kept the pattern and blunted the check for everyone; the new names
are at least as clear — `token_data` for the raw hash from an indexer,
`metadata` for the resolved symbol/name pair, `token_asset` for an
Onchain::Asset in tests.
Verified with the scanner itself rather than by inference: the same pipelock
2.8.0 the workflow pins, run locally over the branch diff, now reports "No
secrets found in diff". It also caught one site the CI log had truncated away
(the system test), which is why the local run was worth setting up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound detection latency, and three review findings
Detection runs on the request thread but asked each candidate chain with a
sync's patience: a 30s timeout, three retries and exponential backoff, per
chain. A 0x address is a candidate on six networks, so one rate-limited
explorer could hold the page for minutes. Detection now reads with its own
budget - one short attempt, no retry, ONCHAIN_DETECTION_TIMEOUT to raise it -
while syncs keep the patient one. A chain that cannot answer in time is
reported as "no activity", which is the screen an ambiguous answer already
produces.
Also from review:
- A full page of Solana signatures is now reported as incomplete history.
The adapter passes no cursor, so a page that fills means the read stopped
short of the address's history; counting only against the transaction
budget called that complete.
- A reused CRYPTO: security with no price provider is bound to the crypto
one. A blank provider falls back to whichever is enabled first, and only
the crypto provider quotes a bare coin symbol, so the holding valued at
zero and read as a broken sync. A provider another integration chose is
left alone: the CRYPTO: prefix is shared with the exchange integrations.
- The chain select's label reached the form builder as an HTML attribute
instead of a label option, so no <label> was associated with the field.
Each regression test was checked against the pre-fix code: the two detection
tests fail with four requests instead of one, and the truncation test fails
by reporting complete history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let the table refuse a token row with no contract
The partial unique indexes key a token on its contract address, and NULLs
are distinct to Postgres, so a token row that reached the table without one
would slip past its index and duplicate freely. The model already refuses
it, but a direct write does not go through the model, and this repo puts
simple guarantees like this in the database.
Added to the existing migration rather than a new one: the table is created
by this branch, so the constraint belongs with it, and this leaves the
schema version untouched.
The regression test writes with validate: false, which is how the sibling
tests prove a guarantee comes from the table rather than from the model
above it. Without the constraint it fails with "expected but nothing was
raised".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): detect a token-only Solana wallet, and pin asset_kind
Two findings from the review of the previous commits.
Detection read only the wallet's lamports, so a wallet emptied of SOL but
still holding SPL tokens answered "nothing here". Each token account carries
its own rent, so an empty wallet address is not an empty wallet. The token
accounts are asked only once the balance comes back zero, so the ordinary
case still costs the one request this probe is meant to be, and accounts
left behind empty do not count as activity.
The narrow blast radius is worth stating: has_activity? only runs when an
address matches more than one chain, and when no candidate answers the user
is asked to choose rather than turned away. So this was a worse screen, not
a rejected wallet.
Separately, the check constraint accepted any asset_kind that carried a
contract address. Each partial unique index names its kind, so a row with
any other one is keyed by nothing and duplicates freely. Adding a token kind
already means adding its index here, so pinning the three in the table adds
no coupling that the indexes did not already have.
Both regression tests fail on the previous code. The third test - emptied
token accounts are not activity - passes either way by design: it guards the
new branch rather than testing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): cover the asset_kind constraint that shipped without it
The constraint landed in
|
||
|
|
6439a731ab |
feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1906)
* feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1481) When the Sidekiq worker container isn't running — the most common Docker Compose misconfiguration in self-hosted setups — every background job silently never executes. Balance calculations, net-worth updates, and account syncs stall. The UI shows zeros and "No balance data available for this date" without explaining why (#1481, #1047). Per jjmata's resolution on the issue, this PR ships both halves of the fix in one pass: 1. A user-facing nudge banner that appears on every authenticated page when Sidekiq isn't processing jobs. Tells the user their data may be stale; doesn't pretend zeros are real. 2. An admin-only deep link from that banner into a new `/settings/admin/system_health` page (super-admin gated, matching the existing admin namespace contract) showing live Sidekiq state: process count, last heartbeat, max queue latency, job counters, and per-queue depth. ## What changed - New `SidekiqHealth` PORO (`app/models/sidekiq_health.rb`) eagerly loads ProcessSet + Queue + Stats in one pass and exposes `healthy?` plus a stable `reason` symbol (`:redis_unreachable`, `:no_worker_processes`, `:stale_heartbeat`, `:queue_backed_up`). Any Redis/Sidekiq failure during the eager load is caught and surfaced as `:redis_unreachable` so a degraded broker never crashes the layout. - `ApplicationController#current_sidekiq_health` memoizes a single instance per request via `helper_method` so the layout, banner partial, and any controller checks share one Redis round-trip. - New `app/views/shared/_sidekiq_health_banner.html.erb` rendered from `_htmldoc.html.erb` when `Current.user` is present and the health check is failing. Banner shows the user-facing message to everyone; the "View system health" CTA + reason detail are gated on `Current.user&.super_admin?`. - New `Admin::SystemHealthController#show` (inherits the existing `Admin::BaseController`, so super-admin gating is enforced for free) + view rendering status, counters, and per-queue breakdown. - Routes: `resource :system_health, only: :show` inside the existing `namespace :admin`. - Settings nav: new "System health" entry under the Advanced section, gated on `super_admin?` to match `sso_providers_label` and `users_label`. - i18n: new `shared.sidekiq_health_banner.*` keys (title, body, CTA, per-reason explanations) and a full `admin.system_health.show.*` namespace for the new admin page. English-only, matching how `ds.pill.*` and other DS keys are scoped. ## Why - jjmata: "Let's take both approaches ... a nudge about 'data unavailable' which hyperlinks to the admin UI if you are an admin only (not for other types of users) sounds like the best path forward. **Any takers for the PR?**" (#1481) - smurfpandey: "We can add a section in Settings for superadmins to see 'health' of the application/host." - The detection signal is conservative on purpose: - `PROCESS_HEARTBEAT_TIMEOUT = 2.minutes` tolerates deploy restarts and brief Redis blips without flapping. - `LATENCY_THRESHOLD = 5.minutes` is well above the sync-job tail under default `config/sidekiq.yml` concurrency. ## Validation This worktree runs on Windows without a local Ruby toolchain, so I could not run `bin/rubocop`, `bundle exec erb_lint`, `bin/brakeman`, or `bin/rails test` locally. CI will run the full matrix on the PR: - `lint` — `bin/rubocop -f github` - `lint_js` — `npm run lint` (no JS touched, should be green) - `scan_ruby` — `bin/brakeman --no-pager` - `scan_js` — `bin/importmap audit` - `test_unit` — `bin/rails test` (includes 7 new tests under `test/models/sidekiq_health_test.rb` and 4 new under `test/controllers/admin/system_health_controller_test.rb`) - `test_system` — `DISABLE_PARALLELIZATION=true bin/rails test:system` - `pipelock` — secret + agent-security diff scan Manual checks done in this worktree: - Re-read `CONTRIBUTING.md` and `.cursor/rules/project-conventions.mdc`. PORO under `app/models/` per Convention 2. No new gem dependency per Convention 1. Banner uses semantic tokens (`bg-warning/10`, `text-warning`) per the design-system rules. No `lucide_icon` direct call — uses the `icon` helper per CLAUDE.md. - Confirmed `Sidekiq::ProcessSet` / `Sidekiq::Queue` / `Sidekiq::Stats` are the same APIs Sidekiq 7+ exposes (we're on Sidekiq 8.x per the `Gemfile.lock` comment in `config/initializers/sidekiq.rb`). - Tests stub `Sidekiq::ProcessSet.new` / `Sidekiq::Queue.all` / `Sidekiq::Stats.new` so the suite doesn't need Redis populated. - The admin route lives inside the existing `namespace :admin` so `Admin::BaseController#require_super_admin!` enforces auth — no new authorization surface added. ## Notes - No public API endpoints, no rswag specs, no OpenAPI changes. - No migrations, no model changes outside the new PORO. - No background jobs touched. - English-only locale entry, mirroring the `ds.*` / `admin.invitations.*` precedent in this repo. Other locales fall back to English. - Detection thresholds are constants on `SidekiqHealth` so they're easy to tune from a follow-up PR if the defaults turn out to flap on any real-world deployment. - The banner positions itself at `top-20` (below the impersonation / super-admin bars) and uses `z-40` (below the `z-50` notification tray). Single-screen overlap with mobile flash toasts is acceptable for V1. Refs: #1481, #1047 * fix(self-hosting): address review on Sidekiq health PR (#1481) - `Admin::SystemHealthController#show` now reads from the request-memoized `current_sidekiq_health` instead of building a fresh `SidekiqHealth.new`, so the controller and the layout banner share one Redis round-trip. - `SidekiqHealth#reason` now treats `last_heartbeat_at.nil?` the same as a stale beat: a registered process that hasn't published a heartbeat is not "healthy". Previously the check short-circuited on the nil guard and silently fell through to the queue-latency branch. Added a unit test covering the `ProcessSet` entry with `"beat" => nil` case. - Settings nav: switched the "System health" entry's icon from `activity` to `heart-pulse` so it no longer duplicates the LLM Usage icon. - Routes: dropped the redundant `controller: "system_health"` option from the `resource :system_health` declaration — Rails infers `Admin::SystemHealthController` from the namespace, matching the style of the sibling `:sso_providers`, `:users`, `:invitations`, and `:families` admin resources. * fix(self-hosting): scope + cache Sidekiq health, admin-only banner (#1481) Addresses the second round of maintainer review on the Sidekiq health PR. - Skip the check entirely in managed mode. `current_sidekiq_health` returns `nil` unless `Rails.application.config.app_mode.self_hosted?`, so authenticated requests in managed deployments add zero Redis round-trips for this feature. - Cache the snapshot across requests via `SidekiqHealth.current` (Rails.cache, TTL `CACHE_TTL` = 60s default, env-overridable). The per-request memoization on `ApplicationController` is preserved on top, so even back-to-back self-hosted pages share one fetch. - Make thresholds operator-tunable. `PROCESS_HEARTBEAT_TIMEOUT`, `LATENCY_THRESHOLD`, and the new `CACHE_TTL` read from `SIDEKIQ_HEALTH_HEARTBEAT_TIMEOUT`, `SIDEKIQ_HEALTH_LATENCY_THRESHOLD`, and `SIDEKIQ_HEALTH_CACHE_TTL` env vars (seconds), with the previous values as defaults. Comments now explain the tuning rationale. - Gate the banner on `Current.user&.super_admin?` at the layout level rather than rendering a vague warning to family members who can't act on it. The partial no longer carries an internal admin check since the call site does it; non-admins see nothing. - Replace the hard-coded `top-20` offset with a computed offset based on which impersonation bars are visible (`top-4` / `top-20` / `top-36`) so the banner doesn't collide with the super-admin or approval bars when both are stacked above it. - `Admin::SystemHealthController#show` now bypasses the cache (`SidekiqHealth.expire_cache!` + `SidekiqHealth.new`) so an operator who just restarted the worker sees fresh state instead of a stale 60-second snapshot. Also lets the page render in managed mode where `current_sidekiq_health` is nil. - Tests: add coverage for `.current` cache reuse and `.expire_cache!` forcing a re-query, swapping `Rails.cache` to a MemoryStore since the test env defaults to `:null_store`. * fix(self-hosting): route singular resource + drop assert_same on cached snapshot (#1481) Two CI failures surfaced once the full pipeline ran on this branch for the first time (it was gated on contributor approval until d04b78e): - Admin system-health controller tests returned 404. Singular `resource :system_health` in `config/routes.rb` makes Rails infer `Admin::SystemHealthsController` (it pluralizes the controller name even for singular resources), but the controller file is named `system_health_controller.rb` / `Admin::SystemHealthController`. Restore the explicit `controller: "system_health"` override that the previous "address review" commit dropped on the (mistaken) premise that Rails would infer it from the namespace — the sibling admin routes all use plural `resources` so they round-trip cleanly, this one doesn't. Comment now spells the gotcha out so the next reviewer doesn't try to "simplify" it again. - `SidekiqHealthTest#test_current_memoizes_across_calls_inside_the_cache_TTL` used `assert_same` on the two returns from `SidekiqHealth.current`. `ActiveSupport::Cache::MemoryStore` defaults to `dup_values: true` and Marshals on read, so a cache hit returns an `==`-equal but `equal?`-different instance. Replace the identity check with the behavioral assertion we actually care about: re-stub `ProcessSet` to raise on the second call, then assert the second `current` return is still healthy (proving Redis was not re-queried). * fix(i18n): drop redundant inline default on system_health nav label (#1481) `system_health_label` is already defined in config/locales/views/settings/en.yml, so the inline `default: "System health"` was a hard-coded English string in the template (DS Drift Patrol Rule 5). Use the bare locale lookup like the sibling nav entries. --------- Co-authored-by: John Baillie <johnbaillie2007@gmail.com> Co-authored-by: Khaostica <256858950+Khaostica@users.noreply.github.com> |
||
|
pro3958andagentloop
|
fcb46be19f |
fix(transactions): re-render form when creating a transaction with no account (#2777)
TransactionsController#create looked up the account with accessible_accounts.find(params.dig(:entry, :account_id)). With no account selected the id is blank, find raises RecordNotFound, and StoreLocation's rescue_from turns that into head :not_found — the 404 on /transactions the reporter saw. Switch to find_by(id:) and, when it returns nil, rebuild the entry, run validation, and re-render :new with 422, matching the existing validation-failure branch. This covers a blank, missing, or invalid account_id, so the user gets the form back with errors instead of a dead button. Fixes #2566 Co-authored-by: agentloop <agentloop@localhost> |
||
|
|
964817748e |
fix: keep excluded entries visible in account activity (#2772)
* fix: keep excluded entries visible in account activity AccountsController#show built the activity list with @account.entries.where(excluded: false), which hard-hid every excluded entry. Trades only appear in the account activity feed, not the global /transactions page, so once a trade was excluded from analytics there was no row to click and no way to toggle exclusion back off. The balance still counted the trade, so the row vanished from the list but remained in the balance tooltip. Use the excluding_split_parents scope instead, matching Transaction::Search. Excluded entries render greyed-out and can be re-included via the existing exclude toggle in the drawer; only split parents stay hidden. The account list is now consistent with both the global transactions list and the balance calculation. Fixes #2612 * Fix account activity test lint --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
rottenit
|
9930b721b3 |
Add inline category creation to transaction form (#3088)
* Add inline category creation to transaction form * Address category selector review feedback * Fix category system test regression * Address category selector review feedback * Use Rails field ID for category selector |
||
|
GFRandGerald
|
07ce130405 |
fix: respect SURE_IMPORT_MAX_NDJSON_SIZE_MB in Sure import GUI upload (#3111)
The GUI upload paths (imports_controller#create_sure_import and Import::UploadsController#update_sure_import_upload) checked file size against the hardcoded SureImport::MAX_NDJSON_SIZE constant instead of SureImport.max_ndjson_size, so self-hosted admins raising SURE_IMPORT_MAX_NDJSON_SIZE_MB had no effect on the GUI — only the API upload paths honored it. Removes the now-unused constant. Fixes #3010. Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |