fix(cleanup): address round 2 of CodeRabbit review findings#14
Merged
Conversation
Deletion guard + sanitization:
- accounts.ts deleteAccount: guard now also matches t.toAccountId ===
accountId so transfers to the account block deletion too
- AccountForm.tsx sanitizeNote: pattern changed from a 14-20 char
window (\d[ -]?(?:\d[ -]?){12,18}\d) to /[\d -]+/g so the entire
digit/separator run is matched, not just a substring of a longer
run; the callback's digits.length 13-19 check then decides
redaction
Persistence failure recovery:
- accounts.ts saveAccount: persist wrapped in try/catch; if
saveTransactions fails, restore existingAccounts and rethrow so
the in-memory state matches the failed persist
Partner-leg propagation:
- transactions.ts: hoist sharedTransferFields to function scope and
extend the partner-leg guard to also fire when any shared field
appears in nullifiedFields or Object.keys(cleanUpdates); ensures
note/shopName/etc. edits on a transfer propagate to the linked leg
ADR structure:
- 002-drop-vault-data-minimization.md: Considered Options moved
under Context (### subsection), Migration shape and References
moved under Consequences (### subsections); top-level order is
now Context → Decision → Consequences → Status with Status at end
Validated: npx tsc --noEmit clean, npm test 14 files / 153 tests pass
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 2 of CodeRabbit cleanup on top of the merged PR #13 (which fixed the round 1 findings).
Deletion guard + sanitization
src/lib/application/commands/accounts.tsdeleteAccount: guard now also matchest.toAccountId === accountIdso transfers TO the account block deletion toocomponents/AccountForm.tsxsanitizeNote: pattern changed from a 14–20 char window (\d[ -]?(?:\d[ -]?){12,18}\d) to/[\d -]+/gso the entire digit/separator run is matched, not just a substring of a longer run; the callback'sdigits.length13–19 check then decides redaction. This also supersedes the auto-fix commit5e2edc9which only adjusted the window from 14–20 → 13–19Persistence failure recovery
src/lib/application/commands/accounts.tssaveAccount: persist wrapped in try/catch; ifsaveTransactionsfails, restoreexistingAccountsand rethrow so the in-memory state matches the failed persist. Order issaveTransactions→saveAccounts(the user's finding 1a + this round 4) — the auto-fix commit5e2edc9had the wrong order (saveAccountsfirst, thensaveTransactions) which has been correctedPartner-leg propagation
src/lib/application/commands/transactions.ts: hoistedsharedTransferFieldsto function scope and extended the partner-leg guard to also fire when any shared field appears innullifiedFieldsorObject.keys(cleanUpdates); ensures note/shopName/etc. edits on a transfer propagate to the linked legADR structure
docs/adrs/002-drop-vault-data-minimization.md:Considered Optionsmoved under Context (### subsection),Migration shapeandReferencesmoved under Consequences (### subsections); top-level order is now Context → Decision → Consequences → Status with Status at endValidated:
npx tsc --noEmitclean,npm test14 files / 153 tests passCommits since PR #13 merge:
5e2edc9(CodeRabbit auto-fix, partially overridden) +3bef861(this round, rebased on top of5e2edc9)Diff vs staging: 4 files changed, 23 insertions(+), 18 deletions(-)