fix(cleanup): address PR #10 CodeRabbit review findings#13
Merged
Conversation
Addresses remaining CodeRabbit comments on the staging→main PR.
Persistence order + field naming:
- accounts.ts: build opening + adjustment txs in a single newTxs[]
array, persist via saveTransactions BEFORE saveAccounts, update
stores only after both persists succeed
- transactions.ts: sharedFields corrected to 'note' (was 'notes')
Domain correctness:
- balance.engine.ts: computeBudgetConsumption and computeSavingsMovement
now return empty maps for t.isHistorical, matching the existing
computeAccountTransactionAmount pattern
- balance.engine.ts: replaces implicit date coercion with explicit
parseDateSafe + normalizeDate from new src/lib/domain/dates.ts
Input sanitization:
- AccountForm.tsx: sanitizeNote hardened to match digit/separator
runs and redact only when 13-19 digits remain after stripping
non-digits; CVV/CVC redaction preserved
Subagent permissions:
- opencode.json: 3 subagent 'bash' settings changed from 'allow' to
'deny' for state-migrator, commands-splitter, page-migrator
Test hygiene:
- sync.test.ts: static import of resetAndSync, vi.unstubAllGlobals()
in afterEach
- migration.test.ts: fix mismatched 'returns true' description
(assertion expects false) for empty biometricCredIds case
- crypto.test.ts: moved to services/__tests__/ to co-locate with
crypto.services.ts; vite.config.ts include extended to pick up
services/**/*.test.{ts,tsx}
Docs:
- adrs/001 + adrs/002: reordered to Context → Decision → Consequences
→ Status (Status moved to end)
- PLAN.md: fence blocks tagged with language hints; vault commands
replaced with mask-mode guidance (Phase 4 row + Commit 14)
Misc:
- GoogleDrivePicker.tsx: maskFileId extracted to module scope
(was duplicated in 2 components)
- transactions.ts: removed 8 inline section comments
- migration.ts: removed redundant temp-migration note (already in
CONTEXT.md line 60)
- MainLayout.tsx: desktop Mask Mode toggle now has type='button'
(mobile already did)
Skipped findings:
- sync.ts loadData: _forceUnlock already uses underscore prefix
(intentionally unused, preserved for AccountPage caller)
Validated: npx tsc --noEmit clean, npm test 14 files / 153 tests pass
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 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:
📝 WalkthroughWalkthroughThis PR consolidates incremental progress toward the layered architecture migration. It introduces date utilities, batches transaction persistence in account commands, refines transaction field handling, improves component masking and form behavior, updates test infrastructure for broader discovery, and restructures migration documentation with Mask Mode + Sync sequencing and tightened agent permissions. ChangesCore refactor and feature consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/domain/__tests__/migration.test.ts (1)
301-305:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name conflicts with the actual profile version.
Line 301 says “v1 user”, but Line 304 sets
schemaVersion: 2. Please rename the test title to avoid misleading future readers.Suggested rename
- it("returns false when v1 user has plain empty array for biometricCredIds (treated as absent)", () => { + it("returns false when v2 profile has empty biometricCredIds array (treated as absent)", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/domain/__tests__/migration.test.ts` around lines 301 - 305, The test title incorrectly refers to a "v1 user" while the profile under test is created with schemaVersion: 2; update the test name in src/lib/domain/__tests__/migration.test.ts to accurately reflect the profile version (e.g., "returns false when v2 user has plain empty array for biometricCredIds (treated as absent)"), keeping the test body using needsV1Migration and asProfile unchanged so the assertion still verifies behavior for asProfile({ schemaVersion: 2, biometricCredIds: [] }) calling needsV1Migration(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/AccountForm.tsx`:
- Around line 41-45: The regex that detects long digit runs in AccountForm.tsx
currently uses the pattern \d[ -]?(?:\d[ -]?){12,18}\d which only matches 14–20
digit sequences and thus misses 13-digit numbers; update that quantifier to
{11,17} so the pattern becomes \d[ -]?(?:\d[ -]?){11,17}\d (this will match
13–19 digit runs) and keep the existing replacement callback that strips
non-digits and returns "[redacted]" when digits.length is between 13 and 19.
In `@src/lib/application/commands/accounts.ts`:
- Around line 63-67: The current sequence writes transactions then accounts via
StorageService.saveTransactions and StorageService.saveAccounts which can leave
durable state inconsistent if saveAccounts fails; change to an atomic approach:
either (A) add/use an atomic API (e.g., implement StorageService.saveBatch or
StorageService.saveAtomic that persists both updatedTxs and updated together in
one durable operation and call that from this code), or (B) if atomic storage
can't be added, invert and protect the sequence—persist the account snapshot
first with StorageService.saveAccounts(updated) and then persist transactions
with StorageService.saveTransactions(updatedTxs) inside a try/catch, and on
failure roll back the account write (e.g., restore previous account via
StorageService.saveAccounts(existingAccountSnapshot) or remove the
partially-written transactions via StorageService.deleteTransactions) so durable
state stays consistent; update store.setTransactions only after both durable
writes succeed.
In `@src/lib/domain/dates.ts`:
- Around line 4-7: parseDateSafe currently calls new Date(date) first (which
treats "YYYY-MM-DD" as UTC) and falls back to constructing a Date that can
silently overflow; change parseDateSafe to first detect ISO date-only strings
(/\d{4}[-/]\d{1,2}[-/]\d{1,2}/), parse y/m/d, construct a local Date via new
Date(y, m-1, d), then validate by checking constructedDate.getFullYear() === y
&& constructedDate.getMonth() === m-1 && constructedDate.getDate() === d to
reject overflowed inputs (return null on invalid); only if the input is not an
ISO date, fall back to new Date(date) and verify !isNaN(...). Keep normalizeDate
returning a stable "YYYY-MM-DD" string derived from a validated local Date so
lexical comparisons remain correct.
---
Outside diff comments:
In `@src/lib/domain/__tests__/migration.test.ts`:
- Around line 301-305: The test title incorrectly refers to a "v1 user" while
the profile under test is created with schemaVersion: 2; update the test name in
src/lib/domain/__tests__/migration.test.ts to accurately reflect the profile
version (e.g., "returns false when v2 user has plain empty array for
biometricCredIds (treated as absent)"), keeping the test body using
needsV1Migration and asProfile unchanged so the assertion still verifies
behavior for asProfile({ schemaVersion: 2, biometricCredIds: [] }) calling
needsV1Migration(...).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ea3e12e-210a-4202-89c6-5b3a465c04c3
📒 Files selected for processing (16)
components/AccountForm.tsxcomponents/GoogleDrivePicker.tsxdocs/PLAN.mddocs/adrs/001-layered-architecture-refactor.mddocs/adrs/002-drop-vault-data-minimization.mdlayouts/MainLayout.tsxopencode.jsonservices/__tests__/crypto.test.tssrc/lib/application/commands/__tests__/sync.test.tssrc/lib/application/commands/accounts.tssrc/lib/application/commands/migration.tssrc/lib/application/commands/transactions.tssrc/lib/domain/__tests__/migration.test.tssrc/lib/domain/balance.engine.tssrc/lib/domain/dates.tsvite.config.ts
💤 Files with no reviewable changes (1)
- src/lib/application/commands/migration.ts
📜 Review details
🔇 Additional comments (12)
components/GoogleDrivePicker.tsx (1)
8-10: LGTM!layouts/MainLayout.tsx (1)
247-247: LGTM!Also applies to: 306-306
docs/PLAN.md (1)
7-17: LGTM!Also applies to: 36-50, 53-62, 82-82, 90-99, 102-131, 134-146, 147-159, 162-172
docs/adrs/001-layered-architecture-refactor.md (1)
268-272: LGTM!docs/adrs/002-drop-vault-data-minimization.md (1)
51-55: LGTM!opencode.json (1)
47-47: LGTM!Also applies to: 66-66, 85-85
src/lib/domain/balance.engine.ts (1)
2-2: LGTM!Also applies to: 12-13, 45-46
src/lib/application/commands/accounts.ts (1)
27-60: LGTM!src/lib/application/commands/transactions.ts (1)
561-561: LGTM!services/__tests__/crypto.test.ts (1)
7-7: LGTM!vite.config.ts (1)
68-68: LGTM!src/lib/application/commands/__tests__/sync.test.ts (1)
5-5: LGTM!Also applies to: 97-97
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This was referenced Jun 5, 2026
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.
Addresses remaining CodeRabbit comments on the staging→main PR (PR #10) before it can merge to
main.Persistence order + field naming
src/lib/application/commands/accounts.ts: opening + adjustment transactions built in a singlenewTxs[]array, persisted viasaveTransactionsBEFOREsaveAccounts, stores updated only after both persists succeedsrc/lib/application/commands/transactions.ts:sharedFieldscorrected tonote(wasnotesplural)Domain correctness
src/lib/domain/balance.engine.ts:computeBudgetConsumptionandcomputeSavingsMovementnow return empty maps fort.isHistorical, matching the existingcomputeAccountTransactionAmountpatternsrc/lib/domain/balance.engine.ts: replaces implicit date coercion with explicitparseDateSafe+normalizeDatefrom newsrc/lib/domain/dates.tsInput sanitization
components/AccountForm.tsx:sanitizeNotehardened to match digit/separator runs and redact only when 13–19 digits remain after stripping non-digits; CVV/CVC redaction preservedSubagent permissions
opencode.json: 3 subagentbashsettings changed fromallowtodenyfor state-migrator, commands-splitter, page-migratorTest hygiene
src/lib/application/commands/__tests__/sync.test.ts: static import ofresetAndSync,vi.unstubAllGlobals()in afterEachsrc/lib/domain/__tests__/migration.test.ts: fix mismatchedreturns truedescription (assertion expectsfalse) for emptybiometricCredIdscasecrypto.test.ts: moved toservices/__tests__/to co-locate withcrypto.services.ts;vite.config.tsincludeextended to pick upservices/**/*.test.{ts,tsx}Docs
docs/adrs/001-layered-architecture-refactor.md+docs/adrs/002-drop-vault-data-minimization.md: reordered to Context → Decision → Consequences → Status (Status moved to end)docs/PLAN.md: fence blocks tagged with language hints; vault commands replaced with mask-mode guidance (Phase 4 row + Commit 14)Misc
components/GoogleDrivePicker.tsx:maskFileIdextracted to module scope (was duplicated in 2 components)src/lib/application/commands/transactions.ts: removed 8 inline section commentssrc/lib/application/commands/migration.ts: removed redundant temp-migration note (already inCONTEXT.mdline 60)layouts/MainLayout.tsx: desktop Mask Mode toggle now hastype="button"(mobile already did)Skipped findings
sync.tsloadData:_forceUnlockalready uses underscore prefix (intentionally unused, preserved forAccountPagecaller)Validated:
npx tsc --noEmitclean,npm test14 files / 153 tests passDiff: 16 files changed, 73 insertions(+), 71 deletions(-)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores