From 170c54db4eaf9c64c3aa4a100bce6226c4c5bfd4 Mon Sep 17 00:00:00 2001 From: notedwin-dev Date: Sat, 6 Jun 2026 03:15:18 +0800 Subject: [PATCH 1/2] fix(cleanup): address PR #10 CodeRabbit review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- components/AccountForm.tsx | 7 +++-- components/GoogleDrivePicker.tsx | 23 +++----------- docs/PLAN.md | 22 +++++++------- .../adrs/001-layered-architecture-refactor.md | 5 +++- docs/adrs/002-drop-vault-data-minimization.md | 5 +++- layouts/MainLayout.tsx | 2 ++ opencode.json | 6 ++-- .../__tests__/crypto.test.ts | 2 +- .../commands/__tests__/sync.test.ts | 4 +-- src/lib/application/commands/accounts.ts | 30 ++++++++++--------- src/lib/application/commands/migration.ts | 3 -- src/lib/application/commands/transactions.ts | 12 +------- src/lib/domain/__tests__/migration.test.ts | 2 +- src/lib/domain/balance.engine.ts | 6 +++- src/lib/domain/dates.ts | 13 ++++++++ vite.config.ts | 2 +- 16 files changed, 73 insertions(+), 71 deletions(-) rename {src/lib/domain => services}/__tests__/crypto.test.ts (98%) create mode 100644 src/lib/domain/dates.ts diff --git a/components/AccountForm.tsx b/components/AccountForm.tsx index 7ace6a7..d6837b7 100644 --- a/components/AccountForm.tsx +++ b/components/AccountForm.tsx @@ -38,8 +38,11 @@ const AccountForm: React.FC = ({ const sanitizeNote = (value: string) => value - .replace(/\b(?=(?:[\s\S]*\d){13,19})(?:\d[ -]?){13,19}\b/g, "[redacted]") - .replace(/\b(cvv|cvc)\s*:?\s*\d{3,4}\b/gi, "[redacted]"); + .replace(/\b(cvv|cvc)\s*:?\s*\d{3,4}\b/gi, "[redacted]") + .replace(/\d[ -]?(?:\d[ -]?){12,18}\d/g, (run) => { + const digits = run.replace(/\D/g, ""); + return digits.length >= 13 && digits.length <= 19 ? "[redacted]" : run; + }); const [confirmationModal, setConfirmationModal] = useState<{ isOpen: boolean; diff --git a/components/GoogleDrivePicker.tsx b/components/GoogleDrivePicker.tsx index 029f4d0..ccd4d5c 100644 --- a/components/GoogleDrivePicker.tsx +++ b/components/GoogleDrivePicker.tsx @@ -5,6 +5,10 @@ import { } from "@googleworkspace/drive-picker-react"; import { logger } from "../src/lib/application/logger"; +const maskFileId = (fileId: string): string => { + return fileId.length > 4 ? `***${fileId.slice(-4)}` : "***"; +}; + interface GoogleDrivePickerProps { onPicked: (fileId: string) => void; onCancel?: () => void; @@ -30,21 +34,6 @@ export const GoogleDrivePicker: React.FC = ({ const apiKey = import.meta.env.VITE_GOOGLE_API_KEY; const appId = import.meta.env.VITE_GOOGLE_APP_ID; - if (!clientId || !apiKey) { - logger.error( - "GoogleDrivePicker: Missing VITE_GOOGLE_CLIENT_ID or VITE_GOOGLE_API_KEY", - ); - } - if (!appId) { - logger.warn( - "GoogleDrivePicker: Missing VITE_GOOGLE_APP_ID - this may cause integration issues", - ); - } - - const maskFileId = (fileId: string): string => { - return fileId.length > 4 ? `***${fileId.slice(-4)}` : "***"; - }; - const handlePicked = (e: CustomEvent) => { const data = e.detail; if (data.docs && data.docs.length > 0) { @@ -124,10 +113,6 @@ export const useGoogleDrivePicker = () => { }); }; - const maskFileId = (fileId: string): string => { - return fileId.length > 4 ? `***${fileId.slice(-4)}` : "***"; - }; - const handlePicked = (e: CustomEvent) => { const data = e.detail; if (data.docs && data.docs.length > 0) { diff --git a/docs/PLAN.md b/docs/PLAN.md index 0d09478..87de311 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -4,7 +4,7 @@ Replace the 2,879-line `DataProvider.tsx` monolith with focused **Zustand stores** (state) and **application commands** (logic). Data flows in one direction: -``` +```text Sheets ──→ DataProvider ──→ Stores ──→ Components │ │ ↓ ↓ @@ -33,7 +33,7 @@ Sheets ──→ DataProvider ──→ Stores ──→ Components ## Architecture -``` +```text src/ ├── lib/ │ ├── domain/ ← Pure functions (balance.engine, currency) @@ -50,7 +50,7 @@ src/ ### Data flow after full migration -``` +```text User action (click "Save") → Command (submitTransaction in commands.ts) → Domain logic (computeAccountTransactionAmount / computeBudgetConsumption / computeSavingsMovement) @@ -79,7 +79,7 @@ Each domain follows the same pattern: | 6 | Pockets | Medium | — | savePocket, deletePocket | AssetsPage | | 7 | Accounts | Medium | — | saveAccount, deleteAccount | AccountCard, AccountForm, AccountPage | | 8 | Transactions | Very High | submit, delete, batchDelete | batchEdit (309 lines), bulkImport (120 lines) | History, Dashboard, Charts | -| 9 | Privacy | Medium | — | vault commands (6 handlers) | Auth, Profile | +| 9 | Mask Mode | Medium | — | mask-mode toggles (replaces vault handlers) | Auth, Profile | | 10 | Sync | Very High | — | syncData (464 lines) | DataProvider itself | | 11 | Cleanup | — | — | — | Remove DataProvider, DataContext, dead code | @@ -87,7 +87,7 @@ Each domain follows the same pattern: ### Phase 1: Foundation (1 commit) -``` +```text Commit 1: DataProvider pipes loaded data into Zustand stores - After loadData completes, call: useFinanceStore.getState().setAccounts(data.accounts) @@ -99,7 +99,7 @@ Commit 1: DataProvider pipes loaded data into Zustand stores ### Phase 2: Simple Domains (8 small commits) -``` +```text Commit 2: Extract Category commands + store wiring - Create saveCategory, deleteCategory in commands.ts - Switch CategoryManager from useData() to useFinanceStore() @@ -131,7 +131,7 @@ Commit 9: Verify all simple domains migrated ### Phase 3: Transactions Domain (3-4 commits, split into sub-tasks) -``` +```text Commit 10: Extract batchEditTransaction to command - 309-line handler in DataProvider → commands.ts - Split into smaller sub-tasks @@ -144,10 +144,10 @@ Commit 12: Switch History page components to useFinanceStore() Commit 13: Switch Dashboard + Charts to useFinanceStore() ``` -### Phase 4: Privacy + Sync (2-3 commits) +### Phase 4: Mask Mode + Sync (2-3 commits) -``` -Commit 14: Create privacy commands (vault enable/disable/lock/unlock) +```text +Commit 14: Create mask-mode commands (toggle setMaskMode, mask helpers) - Switch Auth and Profile components Commit 15: Extract syncData to sync command @@ -159,7 +159,7 @@ Commit 16: Final sync — DataProvider becomes thin ### Phase 5: Cleanup (1-2 commits) -``` +```text Commit 17: Remove DataContext - All components now use stores directly - Delete context/DataContext.tsx diff --git a/docs/adrs/001-layered-architecture-refactor.md b/docs/adrs/001-layered-architecture-refactor.md index 3953dbe..66506b2 100644 --- a/docs/adrs/001-layered-architecture-refactor.md +++ b/docs/adrs/001-layered-architecture-refactor.md @@ -1,6 +1,5 @@ # ADR-001: Layered Architecture Refactor -**Status:** Accepted (in progress) **Date:** 2026-06-01 **Deciders:** Edwin @@ -266,3 +265,7 @@ Pages become thin — they call commands (or use Zustand hooks directly for read - `src/lib/domain/balance.engine.ts` — extracted balance computation - `src/lib/domain/currency.ts` — extracted currency conversion - `src/lib/domain/__tests__/` — 22 tests for domain functions + +--- + +**Status:** Accepted (in progress) diff --git a/docs/adrs/002-drop-vault-data-minimization.md b/docs/adrs/002-drop-vault-data-minimization.md index 7b1d5d7..020843c 100644 --- a/docs/adrs/002-drop-vault-data-minimization.md +++ b/docs/adrs/002-drop-vault-data-minimization.md @@ -1,6 +1,5 @@ # Drop the vault: data minimization for a personal finance tracker -**Status:** Accepted **Date:** 2026-06-04 **Deciders:** Edwin @@ -49,3 +48,7 @@ The only sensitive data the app will hold after this change is whatever the user - `pages/ProfilePage.tsx` — vault UI deleted; export `_note` warning removed - `layouts/MainLayout.tsx` — vault unlock modal deleted - `helpers/useAppInit.tsx` — vault-specific init paths deleted + +--- + +**Status:** Accepted diff --git a/layouts/MainLayout.tsx b/layouts/MainLayout.tsx index 6688dd4..2463908 100644 --- a/layouts/MainLayout.tsx +++ b/layouts/MainLayout.tsx @@ -244,6 +244,7 @@ const MainLayout: React.FC = () => { )}