Release: layered architecture refactor, drop the vault (ADR-002), and Sheets-401 toast fix

[notedwin-dev]

Summary

Promotes staging to main. This release bundles three strands of work plus the bugfix follow-ups from the most recent CodeRabbit review.

Layered architecture refactor (PR #6)

• Domain → application → infrastructure separation enforced
• src/lib/domain/ — pure functions (no React, no I/O): balance.engine.ts, currency.ts, migration.ts, SearchOverlay, matchesSearch
• src/lib/application/commands/ — orchestration, one file per domain (accounts, transactions, sync, goals, subscriptions, pots, pockets, migration, privacy, helpers)
• src/stores/ — Zustand: finance.store, sync.store, mask.store, ui.store
• DataProvider / DataContext removed; context/ directory deleted
• Path alias src/ configured in tsconfig.json + vite.config.ts
• Test framework: vitest + jsdom for stores, plain vitest for domain; 127 tests pass

Drop the vault (ADR-002, PR #8)

• Account type drops cardNumber, cvv, expiry, holderName, accountNumber, details, isEncrypted
• Profile type drops totpSecret, vaultSalt, isSecurityEnabled, isVaultLocked, biometricCredId(s)
• Account.note (freeform unencrypted) added; Profile.schemaVersion: 2 added
• usePrivacyStore → useMaskStore (UI-only toggle renamed)
• Services deleted: security.services.ts, twofa.services.ts, biometric.services.ts
• Migration code (migration.ts) is temporary — self-healing, idempotent, hard v1→v2 cleanup; user confirms removal once both devices are on v2
• Cloud cleanup: sheets.services.ts filters legacy sensitive fields on save; Profile sheet header expanded to A1:J1
• Stale TransactionForm.tsx:344 comment ("Will be updated by DataProvider logic") left as-is (no functional impact)

Stuck Sheets-toast fix (PR #9)

• resetAndSync + syncData now re-throw 401 in three places in sheets.services.ts; non-401 errors fall through to "Working offline" path
• getSpreadsheetId saved-ID catch: rethrows 401 with clearGapiAccessToken() so token-clearing is consistent with the drive.files.list path
• auth.services.tsx updateProfile uses updatedP!.email instead of stale profile.email closure for the cloud write
• findUser rethrows 401 (other errors still return null) so handleGoogleSuccess can distinguish auth failure from "user not found" and not create a duplicate row
• Toast auto-dismiss (3s) restored in useSyncStore.showToast (regression from PR Layered Architecture: Eliminate DataProvider bottleneck, finalize ADR-001 migration #6)
• sanitizeNote regex scoped to PAN-length (13–19 digit word-boundary) + explicit CVV/CVC labels

Follow-ups / CodeRabbit review

• 10 of 13 CodeRabbit findings applied to drop-vault PR
• 5 follow-up findings applied to sync-toast PR (the 6th — account.note masking in AccountPage — applied to feature/drop-vault)

Fixed Issues

Closes #1 — Refactor: Rename and split computeBalanceDeltas into three named functions
Closes #3 — Privacy store: migrate masking and vault from DataProvider (mask part done; vault part superseded by #7)
Closes #4 — Commands: extract remaining handlers from DataProvider
Closes #5 — Cleanup: remove DataProvider and DataContext
Closes #7 — Drop the vault: data minimization for a personal finance tracker

Verification

• npx tsc --noEmit — clean
• npm run test — 11 test files, 127 tests pass
• Manual end-to-end smoke (staging): see Issue Drop the vault: data minimization for a personal finance tracker #7's "End-to-end smoke" section

Follow-ups (post-merge)

• Apply patched backend-index.js for CORS fix in finance-tracker-app-backendS (separate repo) + redeploy
• After user confirms v1→v2 cutover is complete: delete the migrate button block + runVaultSchemaMigration imports in Profile.tsx, then delete src/lib/application/commands/migration.ts and src/lib/domain/migration.ts
• Consider parity for desktop Mask Mode toggle aria-label / aria-pressed (only the mobile button was in the CodeRabbit findings list)
• Clean up the stale comment in components/TransactionForm.tsx:344

Summary by CodeRabbit

• New Features

  • Mask Mode toggle to hide amounts/text; hover-to-reveal behavior
  • Notes field for accounts and simplified account details
  • App init now runs background data sync and migration tasks

• Improvements

  • Removed legacy vault/biometric flows and sensitive-field UI
  • Store/command-driven state with streamlined balance recalculation
  • Centralized, redacting logger and small accessibility tweaks

• Chores

  • One-time v1→v2 data migration and extensive docs/architecture updates

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

vite.config.ts (1)

64-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Domain tests should run without jsdom per coding guidelines.

The global environment: "jsdom" applies to all tests including those in src/lib/domain/__tests__/. This violates the guideline that domain tests should run without jsdom to ensure domain functions remain pure and don't accidentally depend on browser APIs.

Use environmentMatchGlobs to run domain tests in the node environment:

Suggested fix

 test: {
   globals: true,
   environment: "jsdom",
   setupFiles: ["./src/test-setup.ts"],
   include: ["src/**/*.test.{ts,tsx}"],
+  environmentMatchGlobs: [
+    ["src/lib/domain/**/*.test.ts", "node"],
+  ],
 },

🤖 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 `@vite.config.ts` around lines 64 - 69, The global test config currently sets
environment: "jsdom" for all tests; update the Vite/Vitest test config (the test
object) to keep environment: "jsdom" as default but add environmentMatchGlobs to
override domain tests to node (e.g., match "src/lib/domain/**" or
"src/lib/domain/**/__tests__/**" to run in node). Modify the test config entry
named test to include environmentMatchGlobs with a pattern targeting the domain
test files so those tests run in the "node" environment while leaving other
tests using "jsdom".

services/auth.services.tsx (2)

256-260: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear all OAuth tokens on logout.

google_access_token and google_token_expiry remain in localStorage after logout, so credentials are retained longer than expected.

Suggested fix

 	const logout = () => {
 		googleLogout();
 		SheetService.clearGapiAccessToken();
+		localStorage.removeItem("google_access_token");
+		localStorage.removeItem("google_token_expiry");
 		localStorage.removeItem("google_refresh_token");
 		const emptyProfile: UserProfile = {
 			name: "",

🤖 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 `@services/auth.services.tsx` around lines 256 - 260, The logout function
currently calls googleLogout() and clears the refresh token but leaves
google_access_token and google_token_expiry in localStorage; update the logout
implementation (function logout) to also remove "google_access_token" and
"google_token_expiry" from localStorage and ensure
SheetService.clearGapiAccessToken() is called to revoke in-memory GAPI state so
all OAuth credentials are cleared on logout.

---

286-297: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix use of setProfile updater side effects in updateProfile so updatedP is always defined.

In services/auth.services.tsx (lines 286-297), updatedP is assigned inside the setProfile(prev => { ... }) updater, but then immediately read afterward (updatedP.email). The updater execution isn’t guaranteed to happen before the following conditional runs, so updatedP can be undefined. Compute the merged “next profile” synchronously first (and then use that same value for StorageService.saveProfile and SheetService.updateUser) rather than relying on the updater side effect.

🤖 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 `@services/auth.services.tsx` around lines 286 - 297, In updateProfile, avoid
relying on setProfile's updater side-effects: compute the merged next profile
synchronously (e.g., const nextProfile = { ...currentProfile, ...u }) before
calling setProfile, then call setProfile(nextProfile),
StorageService.saveProfile(nextProfile), and use nextProfile when checking
nextProfile.email and calling SheetService.updateUser; keep the existing guards
(skipCloud and SheetService.isClientReady()) and await
SheetService.updateUser(nextProfile.email, u) as before.

services/sheets.services.ts (1)

306-326: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Header migration may cause infinite recursion if sheet write fails.

If the update call at line 318-323 succeeds but a subsequent re-fetch in the recursive findUser(email) call still reports missing headers (e.g., due to eventual consistency or a race), this could loop. Consider adding a guard parameter to limit recursion depth.

♻️ Proposed fix

-export const findUser = async (email: string) => {
+export const findUser = async (email: string, _migrationAttempted = false): Promise<any> => {
 	if (!gapiInited || !hasAccessToken) return null;
 	try {
 		// ... existing code ...
 
 		const missingHeaders = requiredHeaders.filter((h) => !headers.includes(h));
-		if (missingHeaders.length > 0) {
+		if (missingHeaders.length > 0 && !_migrationAttempted) {
 			// ... update headers ...
-			return findUser(email);
+			return findUser(email, true);
 		}
+		if (missingHeaders.length > 0 && _migrationAttempted) {
+			logger.warn("Header migration did not persist, proceeding with available headers");
+		}

🤖 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 `@services/sheets.services.ts` around lines 306 - 326, The header-migration
path can recurse indefinitely via the findUser(email) call after calling
window.gapi.client.sheets.spreadsheets.values.update; add a guard (e.g., an
optional retryCount or maxRetries parameter) to the findUser function and use it
when re-fetching so that after N attempts you stop retrying (return a clear
error/partial result or throw), update the call site here to pass/increment that
retry counter, and keep references to requiredHeaders, getProfileSheetName,
getColumnLetter and the update call when implementing the guard to prevent
infinite recursion if the sheet write is not visible immediately.

♻️ Duplicate comments (1)

pages/ProfilePage.tsx (1)

42-46: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Account sanitization still incomplete per ADR-002.

The current sanitizer only strips details and isEncrypted, but legacy accounts may contain other sensitive fields (cardNumber, cvv, expiry, holderName, accountNumber). This matches the concern raised in a previous review.

🐛 Proposed fix aligning with sheets.services.ts sensitiveFields

     const sanitizedAccounts = accounts.map((a) => {
-      const bag = a as unknown as Record<string, unknown>;
-      const { details, isEncrypted, ...rest } = bag;
-      return rest;
+      const sensitiveKeys = new Set([
+        "details",
+        "isEncrypted",
+        "accountNumber",
+        "cardNumber",
+        "holderName",
+        "expiry",
+        "cvv",
+      ]);
+      return Object.fromEntries(
+        Object.entries(a as unknown as Record<string, unknown>).filter(
+          ([key]) => !sensitiveKeys.has(key),
+        ),
+      );
     });

🤖 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 `@pages/ProfilePage.tsx` around lines 42 - 46, The sanitizer in the
accounts.map (sanitizedAccounts) only removes details and isEncrypted; update it
to also strip all legacy sensitive fields (e.g., cardNumber, cvv, expiry,
holderName, accountNumber) before returning each account. Locate the
accounts.map callback (const bag = a as unknown as Record<string, unknown>;
const { details, isEncrypted, ...rest } = bag;) and extend the destructuring or
explicitly delete those sensitive keys from bag/rest so the returned object
excludes them, matching the sensitiveFields set used in sheets.services.ts.

🤖 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 39-42: The current sanitizeNote() regex uses a global lookahead
that depends on digit distribution and the proposed simpler pattern changes
behavior; instead, update sanitizeNote to find candidate runs matching /(?:\d[
-]?){13,19}/ (or similar), then for each match strip non-digits and check the
digit-count is between 13 and 19 before replacing with "[redacted]"; also keep
the existing CVV/CVC regex logic in sanitizeNote to redact 3–4 digit codes. This
ensures strict 13–19 digit redaction without the original lookahead’s
cross-string sensitivity while preserving the sanitizeNote function name and
intent.

In `@components/GoogleDrivePicker.tsx`:
- Around line 44-46: Duplicate maskFileId functions exist inside the
GoogleDrivePicker component and a hook; extract a single typed
maskFileId(fileId: string): string to module scope (top of the file), remove the
duplicate implementations, and update both the GoogleDrivePicker component and
the hook to call the shared maskFileId function so they use the single
implementation; ensure the signature remains the same and adjust any
imports/exports only if the hook is moved to a different module.

In `@src/lib/application/commands/__tests__/sync.test.ts`:
- Line 88: The test suite stubs the global confirm with vi.stubGlobal("confirm",
...) in beforeEach but never restores it; add a restoration in afterEach using
vi.unstubAllGlobals() (or explicitly unstub the confirm global) alongside the
existing vi.useRealTimers() so the global stub doesn't leak into other suites;
locate the beforeEach/afterEach in the sync.test.ts file and update the
afterEach to call vi.useRealTimers() followed by vi.unstubAllGlobals().

In `@src/lib/domain/__tests__/crypto.test.ts`:
- Around line 1-8: The test file that imports hashPassword, verifyPassword,
isLegacyHash and PBKDF2_ITERATIONS is placed in the domain tests folder but
targets services/crypto.services.ts—move the test file so it is co-located with
the module under test (i.e., place the test next to crypto.services.ts in the
services tests directory), rename it to crypto.services.test.ts (or similar) to
match the module, and update any import paths so imports for hashPassword,
verifyPassword, isLegacyHash and PBKDF2_ITERATIONS resolve correctly.

---

Outside diff comments:
In `@services/auth.services.tsx`:
- Around line 256-260: The logout function currently calls googleLogout() and
clears the refresh token but leaves google_access_token and google_token_expiry
in localStorage; update the logout implementation (function logout) to also
remove "google_access_token" and "google_token_expiry" from localStorage and
ensure SheetService.clearGapiAccessToken() is called to revoke in-memory GAPI
state so all OAuth credentials are cleared on logout.
- Around line 286-297: In updateProfile, avoid relying on setProfile's updater
side-effects: compute the merged next profile synchronously (e.g., const
nextProfile = { ...currentProfile, ...u }) before calling setProfile, then call
setProfile(nextProfile), StorageService.saveProfile(nextProfile), and use
nextProfile when checking nextProfile.email and calling SheetService.updateUser;
keep the existing guards (skipCloud and SheetService.isClientReady()) and await
SheetService.updateUser(nextProfile.email, u) as before.

In `@services/sheets.services.ts`:
- Around line 306-326: The header-migration path can recurse indefinitely via
the findUser(email) call after calling
window.gapi.client.sheets.spreadsheets.values.update; add a guard (e.g., an
optional retryCount or maxRetries parameter) to the findUser function and use it
when re-fetching so that after N attempts you stop retrying (return a clear
error/partial result or throw), update the call site here to pass/increment that
retry counter, and keep references to requiredHeaders, getProfileSheetName,
getColumnLetter and the update call when implementing the guard to prevent
infinite recursion if the sheet write is not visible immediately.

In `@vite.config.ts`:
- Around line 64-69: The global test config currently sets environment: "jsdom"
for all tests; update the Vite/Vitest test config (the test object) to keep
environment: "jsdom" as default but add environmentMatchGlobs to override domain
tests to node (e.g., match "src/lib/domain/**" or
"src/lib/domain/**/__tests__/**" to run in node). Modify the test config entry
named test to include environmentMatchGlobs with a pattern targeting the domain
test files so those tests run in the "node" environment while leaving other
tests using "jsdom".

---

Duplicate comments:
In `@pages/ProfilePage.tsx`:
- Around line 42-46: The sanitizer in the accounts.map (sanitizedAccounts) only
removes details and isEncrypted; update it to also strip all legacy sensitive
fields (e.g., cardNumber, cvv, expiry, holderName, accountNumber) before
returning each account. Locate the accounts.map callback (const bag = a as
unknown as Record<string, unknown>; const { details, isEncrypted, ...rest } =
bag;) and extend the destructuring or explicitly delete those sensitive keys
from bag/rest so the returned object excludes them, matching the sensitiveFields
set used in sheets.services.ts.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a7f3a6eb-1588-4a34-a257-4f54a8d9a30f

📥 Commits

Reviewing files that changed from the base of the PR and between 63e92e0 and f3edfb7.

📒 Files selected for processing (22)

• components/AIInsights.tsx
• components/AccountForm.tsx
• components/BulkImportModal.tsx
• components/GoogleDrivePicker.tsx
• components/Profile.tsx
• components/history/useFilteredTransactions.ts
• helpers/useAppInit.tsx
• pages/ProfilePage.tsx
• services/auth.services.tsx
• services/coin.services.ts
• services/crypto.services.ts
• services/exchange.services.ts
• services/gemini.services.ts
• services/sheets.services.ts
• services/storage.services.ts
• src/lib/application/__tests__/logger.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/sync.ts
• src/lib/application/logger.ts
• src/lib/domain/__tests__/crypto.test.ts
• vite.config.ts

📜 Review details

🧰 Additional context used

📓 Path-based instructions (6)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid comments — prefer self-documenting code with clear names and structure

Files:

• services/exchange.services.ts
• components/AIInsights.tsx
• components/BulkImportModal.tsx
• services/coin.services.ts
• src/lib/application/__tests__/logger.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• components/history/useFilteredTransactions.ts
• src/lib/application/logger.ts
• services/gemini.services.ts
• src/lib/domain/__tests__/crypto.test.ts
• pages/ProfilePage.tsx
• components/GoogleDrivePicker.tsx
• services/crypto.services.ts
• services/storage.services.ts
• src/lib/application/commands/migration.ts
• helpers/useAppInit.tsx
• vite.config.ts
• src/lib/application/commands/sync.ts
• services/auth.services.tsx
• components/AccountForm.tsx
• components/Profile.tsx
• services/sheets.services.ts

{components,pages,layouts}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Tailwind CSS utility-first approach for styling

React components should read state from useFinanceStore instead of legacy useData() context

Use domain commands exported from src/lib/application/commands.ts for state mutations instead of handler callbacks

Files:

• components/AIInsights.tsx
• components/BulkImportModal.tsx
• components/history/useFilteredTransactions.ts
• pages/ProfilePage.tsx
• components/GoogleDrivePicker.tsx
• components/AccountForm.tsx
• components/Profile.tsx

src/lib/application/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Application layer should orchestrate commands without side effects, delegating to domain and infrastructure layers

Files:

• src/lib/application/__tests__/logger.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/logger.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/sync.ts

src/lib/domain/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Domain layer functions must be pure functions with no React, no side effects, and no imports from context/, services/, or stores/

All domain layer dependencies must be explicit parameters

Use TransactionType enum from types for type discrimination in domain logic

Use one module per file in the domain layer with co-located tests in __tests__/

Files:

• src/lib/domain/__tests__/crypto.test.ts

src/lib/domain/__tests__/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Co-locate domain layer unit tests in __tests__/ directories next to the module being tested

Files:

• src/lib/domain/__tests__/crypto.test.ts

src/lib/domain/__tests__/**/*.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

Write pure unit tests for domain functions using Vitest without jsdom

Files:

• src/lib/domain/__tests__/crypto.test.ts

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T16:24:37.064Z
Learning: Create feature branches with prefix `feature/*` for new features (e.g., `feature/export-csv`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T16:24:37.064Z
Learning: Create refactor branches with prefix `refactor/*` for major code refactoring or UI revamps (e.g., `refactor/layered-architecture`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T16:24:37.064Z
Learning: Create bugfix branches with prefix `bugfix/*` for bug fixes (e.g., `bugfix/search-overlay-crash`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T16:24:37.064Z
Learning: Open pull requests to `staging` branch for review before merging to `main`

🔇 Additional comments (25)

vite.config.ts (2)

8-20: LGTM!

---

22-63: LGTM!

src/lib/application/logger.ts (1)

1-109: LGTM!

src/lib/application/__tests__/logger.test.ts (1)

1-109: LGTM!

src/lib/application/commands/migration.ts (1)

8-8: LGTM!

Also applies to: 39-42

src/lib/application/commands/sync.ts (1)

9-9: LGTM!

Also applies to: 42-55, 107-107, 162-164, 171-171, 203-206, 240-242, 290-292, 309-309, 364-368, 402-402, 569-569

helpers/useAppInit.tsx (1)

14-14: LGTM!

Also applies to: 33-33, 72-72

services/coin.services.ts (1)

1-1: LGTM!

Also applies to: 44-44

services/auth.services.tsx (1)

9-10: LGTM!

Also applies to: 60-60, 163-168, 193-200, 299-302

services/crypto.services.ts (1)

1-135: LGTM!

services/exchange.services.ts (1)

2-2: LGTM!

Also applies to: 47-47, 151-151

services/gemini.services.ts (1)

12-12: LGTM!

Also applies to: 289-289, 408-417, 455-455, 495-499, 565-565

services/storage.services.ts (1)

14-14: LGTM!

Also applies to: 82-83, 90-90, 163-163, 167-167

components/AIInsights.tsx (1)

18-18: LGTM!

Also applies to: 376-376

components/BulkImportModal.tsx (1)

18-18: LGTM!

Also applies to: 106-106

services/sheets.services.ts (3)

29-35: LGTM!

---

57-68: LGTM!

Also applies to: 97-97, 124-124, 149-159, 211-214, 231-231, 248-253, 264-269, 283-283, 462-462, 500-511, 533-548, 675-678, 771-778, 825-828, 877-880, 969-977, 1004-1004, 1061-1067, 1131-1134, 1180-1191, 1316-1316, 1373-1375, 1402-1402

---

694-709: Sensitive fields set should include note for v1 accounts if it contained sensitive data.

The sensitiveFields set correctly strips legacy vault fields. However, per the context snippet from sync.ts and ADR-002, this list appears complete for the v1→v2 migration. The implementation looks correct.

LGTM!

src/lib/domain/__tests__/crypto.test.ts (2)

9-93: LGTM!

---

95-101: LGTM!

components/AccountForm.tsx (1)

59-100: LGTM!

Also applies to: 102-128, 130-183, 185-380

components/GoogleDrivePicker.tsx (1)

1-6: LGTM!

Also applies to: 20-105, 107-177

components/Profile.tsx (1)

28-28: LGTM!

Also applies to: 63-72, 287-310, 394-394, 436-472

components/history/useFilteredTransactions.ts (1)

9-9: LGTM!

Also applies to: 64-71

pages/ProfilePage.tsx (1)

9-9: LGTM!

Also applies to: 48-67, 98-113, 115-125

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

components/History.tsx (2)

174-188: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Remove stable imports from dependency arrays.

batchDeleteTransaction is a module-level import, not a reactive value. Including it in the dependency array is unnecessary and slightly misleading.

♻️ Proposed fix

-	}, [batch, batchDeleteTransaction, accounts, pots, pockets, usdRate, transactions]);
+	}, [batch, accounts, pots, pockets, usdRate, transactions]);

🤖 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 `@components/History.tsx` around lines 174 - 188, The useCallback for
handleBatchDelete incorrectly includes the module-level import
batchDeleteTransaction in its dependency array; remove batchDeleteTransaction
from the dependency array and leave only reactive values (batch, accounts, pots,
pockets, usdRate, transactions) so the hook dependencies reflect actual reactive
inputs used by handleBatchDelete (function name: handleBatchDelete, imported
symbol: batchDeleteTransaction).

---

190-206: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Remove stable import from dependency array.

Same as above—batchEditTransactions is a module-level import.

♻️ Proposed fix

-	}, [batch, batchEditTransactions, transactions, accounts, pots, pockets, usdRate]);
+	}, [batch, transactions, accounts, pots, pockets, usdRate]);

🤖 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 `@components/History.tsx` around lines 190 - 206, The useCallback for
handleBatchEditSubmit incorrectly includes the module-level import
batchEditTransactions in its dependency array; remove batchEditTransactions from
the dependency array of handleBatchEditSubmit and leave only reactive values
(batch, transactions, accounts, pots, pockets, usdRate) so the callback doesn't
re-create unnecessarily; update the dependency list for handleBatchEditSubmit
accordingly in the component where handleBatchEditSubmit is declared.

♻️ Duplicate comments (1)

layouts/MainLayout.tsx (1)

246-249: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add aria-label to match mobile toggle.

The mobile mask-mode button (line 306) has both aria-label and aria-pressed. The desktop toggle now has aria-pressed but is missing aria-label for screen-reader parity.

♿ Proposed fix

           <button
             onClick={() => setMaskMode(!maskMode)}
+            aria-label="Toggle mask mode"
             aria-pressed={maskMode}
             className="w-full flex items-center justify-between p-3 rounded-xl bg-gray-900/50 hover:bg-gray-800/50 border border-gray-800/50 transition-all group"
           >

🤖 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 `@layouts/MainLayout.tsx` around lines 246 - 249, The desktop mask-mode toggle
button that calls setMaskMode(!maskMode) currently has aria-pressed but is
missing an aria-label; add an aria-label to that same button element (the one
using onClick={() => setMaskMode(!maskMode)} and aria-pressed={maskMode}) and
use the same label text as the mobile mask-mode button so screen readers get
parity between toggles.

🤖 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.

Outside diff comments:
In `@components/History.tsx`:
- Around line 174-188: The useCallback for handleBatchDelete incorrectly
includes the module-level import batchDeleteTransaction in its dependency array;
remove batchDeleteTransaction from the dependency array and leave only reactive
values (batch, accounts, pots, pockets, usdRate, transactions) so the hook
dependencies reflect actual reactive inputs used by handleBatchDelete (function
name: handleBatchDelete, imported symbol: batchDeleteTransaction).
- Around line 190-206: The useCallback for handleBatchEditSubmit incorrectly
includes the module-level import batchEditTransactions in its dependency array;
remove batchEditTransactions from the dependency array of handleBatchEditSubmit
and leave only reactive values (batch, transactions, accounts, pots, pockets,
usdRate) so the callback doesn't re-create unnecessarily; update the dependency
list for handleBatchEditSubmit accordingly in the component where
handleBatchEditSubmit is declared.

---

Duplicate comments:
In `@layouts/MainLayout.tsx`:
- Around line 246-249: The desktop mask-mode toggle button that calls
setMaskMode(!maskMode) currently has aria-pressed but is missing an aria-label;
add an aria-label to that same button element (the one using onClick={() =>
setMaskMode(!maskMode)} and aria-pressed={maskMode}) and use the same label text
as the mobile mask-mode button so screen readers get parity between toggles.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 604ee940-4633-457f-be89-3b3e20c98755

📥 Commits

Reviewing files that changed from the base of the PR and between f3edfb7 and 3f81a18.

📒 Files selected for processing (25)

• .opencode/prompts/page-migrator.txt
• .opencode/prompts/state-migrator.txt
• AGENTS.md
• components/History.tsx
• components/history/useFilteredTransactions.ts
• docs/adrs/001-layered-architecture-refactor.md
• docs/issues/003-privacy-store.md
• docs/issues/004-commands-extraction.md
• docs/issues/005-cleanup.md
• docs/issues/review-pr2.md
• helpers/useAppInit.tsx
• layouts/MainLayout.tsx
• opencode.json
• pages/ProfilePage.tsx
• src/lib/application/commands/balance.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/commands/pots.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/application/commands/sync.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/migration.ts
• src/lib/domain/search.ts

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

{src,components,pages,helpers,layouts,services}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid comments in code—prefer self-documenting code with clear naming and structure.

Files:

• src/lib/application/commands/pots.ts
• src/lib/domain/search.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• helpers/useAppInit.tsx
• src/lib/application/commands/balance.ts
• pages/ProfilePage.tsx
• src/lib/domain/migration.ts
• src/lib/domain/__tests__/migration.test.ts
• components/History.tsx
• src/lib/application/commands/sync.ts
• components/history/useFilteredTransactions.ts
• layouts/MainLayout.tsx

src/lib/application/commands/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Command handlers in src/lib/application/commands/ should orchestrate domain logic and coordinate with stores. Each command should perform a single action (e.g., save, delete) and delegate business logic to domain functions.

Files:

• src/lib/application/commands/pots.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/application/commands/balance.ts
• src/lib/application/commands/sync.ts

src/lib/domain/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Domain layer functions must be pure functions with no React dependencies, no side effects, and no imports from services/ or stores/. All dependencies must be explicit parameters.

Use TransactionType enum from types for type discrimination in domain logic.

Files:

• src/lib/domain/search.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• src/lib/domain/migration.ts
• src/lib/domain/__tests__/migration.test.ts

**/__tests__/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Co-locate unit tests in __tests__/ directories next to the module being tested. Domain functions use pure unit tests (Vitest, no jsdom). Zustand stores use store action tests (Vitest + jsdom). React components use component tests (Vitest + React Testing Library + jsdom).

Files:

• src/lib/domain/__tests__/matchesSearch.test.ts
• src/lib/domain/__tests__/migration.test.ts

{components,pages}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

State management lives in Zustand stores under src/stores/. React components must never call storage or sheet services directly—they must go through command handlers in src/lib/application/commands/.

Files:

• pages/ProfilePage.tsx
• components/History.tsx
• components/history/useFilteredTransactions.ts

{components,pages,layouts}/**/*.tsx

📄 CodeRabbit inference engine (AGENTS.md)

Use Tailwind CSS for styling with a utility-first approach.

Files:

• pages/ProfilePage.tsx
• components/History.tsx
• layouts/MainLayout.tsx

{components,pages}/**/*.tsx

📄 CodeRabbit inference engine (AGENTS.md)

React components should read goals, subscriptions, and chatSessions from useFinanceStore and call domain commands like saveGoal, deleteGoal, addSubscription, deleteSubscription, saveChatSession, and deleteChatSession instead of using DataContext handlers.

React components should read pots, pockets, and accounts from useFinanceStore and call domain commands like savePot, deletePot, saveSavingPocket, deleteSavingPocket, saveAccount, and deleteAccount instead of using DataContext handlers.

Files:

• pages/ProfilePage.tsx
• components/History.tsx

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T18:25:25.897Z
Learning: Create Git branches with prefixes: `feature/*` for new features, `refactor/*` for major code refactoring or UI revamp, `bugfix/*` for bug fixes. Use kebab-case for branch names.

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T18:25:25.897Z
Learning: Push feature/refactor/bugfix branches to `staging` for review before merging into `main`. `main` auto-deploys to Vercel.

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T18:25:25.897Z
Learning: Document significant architectural decisions in Architecture Decision Records (ADRs) in `docs/adrs/` using the template: Context → Decision → Consequences → Status. Number sequentially and use kebab-case filenames.

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T18:25:25.897Z
Learning: Environment variables must be prefixed with `VITE_` for frontend use: `VITE_BACKEND_API_URL` (backend URL, empty = Vite proxy), `VITE_GEMINI_API_KEY` (Gemini API key), `VITE_GOOGLE_API_KEY` (Google API key).

🔇 Additional comments (25)

src/lib/application/commands/balance.ts (1)

73-77: LGTM!

src/lib/application/commands/chat-sessions.ts (1)

17-17: LGTM!

Also applies to: 30-30

src/lib/application/commands/migration.ts (1)

18-18: LGTM!

src/lib/application/commands/pockets.ts (1)

24-24: LGTM!

Also applies to: 40-45

src/lib/application/commands/pots.ts (1)

26-26: LGTM!

Also applies to: 39-39

src/lib/application/commands/subscriptions.ts (1)

22-22: LGTM!

Also applies to: 35-35

src/lib/application/commands/sync.ts (1)

547-547: LGTM!

src/lib/domain/__tests__/migration.test.ts (2)

301-308: Test description still contradicts assertion.

The test title says "returns true" but the assertion expects false. This was flagged in a previous review and remains unfixed.

📝 Proposed fix

-  it("returns true when v1 user has plain empty array for biometricCredIds (treated as absent)", () => {
+  it("returns false when v1 user has plain empty array for biometricCredIds (treated as absent)", () => {

---

177-188: LGTM!

Also applies to: 196-196, 212-212, 218-218

components/History.tsx (1)

158-172: LGTM!

components/history/useFilteredTransactions.ts (1)

9-12: LGTM!

helpers/useAppInit.tsx (1)

40-54: LGTM!

pages/ProfilePage.tsx (1)

42-58: LGTM!

src/lib/domain/__tests__/matchesSearch.test.ts (1)

1-5: LGTM!

Also applies to: 92-99

docs/issues/003-privacy-store.md (1)

24-24: Use a non-brittle verification gate instead of fixed test counts.

The hardcoded test count is likely stale; prefer wording like “All tests pass” to keep this checklist durable.

docs/issues/004-commands-extraction.md (1)

29-29: Avoid pinning verification to a fixed test count.

Use a stable criterion (e.g., “All tests pass”) so this issue doc does not go stale.

AGENTS.md (1)

79-79: LGTM!

Also applies to: 97-97

docs/adrs/001-layered-architecture-refactor.md (1)

113-113: LGTM!

src/lib/domain/migration.ts (1)

58-58: LGTM!

Also applies to: 63-63

src/lib/domain/search.ts (1)

1-31: LGTM!

docs/issues/005-cleanup.md (1)

16-19: LGTM!

Also applies to: 26-26

docs/issues/review-pr2.md (1)

3-3: LGTM!

.opencode/prompts/page-migrator.txt (1)

3-3: LGTM!

Also applies to: 6-6, 8-11

.opencode/prompts/state-migrator.txt (1)

1-3: LGTM!

Also applies to: 6-11

opencode.json (1)

4-4: LGTM!

[notedwin-dev]

@coderabbitai what is not fixed yet? List the issues and prompt

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="notedwin-dev/finance-tracker-app"
PR=10

echo "=== PR basic info ==="
gh pr view "$PR" --repo "$REPO" --json number,title,state,mergeable,reviewDecision,url,comments,reviews,files --jq '{
  number,title,state,mergeable,reviewDecision,url,
  commentsCount:(.comments|length),
  reviewsCount:(.reviews|length),
  filesCount:(.files|length)
}'

echo
echo "=== Review comments by coderabbitai / CodeRabbit-like bots ==="
gh api "repos/$REPO/pulls/$PR/comments?per_page=100" --paginate \
  --jq '.[] | select((.user.login|ascii_downcase) | test("coderabbit")) | {
    id,
    user:.user.login,
    path,
    line,
    original_line,
    position,
    in_reply_to_id,
    outdated,
    url:.html_url,
    body
  }' || true

echo
echo "=== Top-level issue comments by coderabbitai / CodeRabbit-like bots ==="
gh api "repos/$REPO/issues/$PR/comments?per_page=100" --paginate \
  --jq '.[] | select((.user.login|ascii_downcase) | test("coderabbit")) | {
    id,
    user:.user.login,
    created_at,
    updated_at,
    url:.html_url,
    body
  }' || true

echo
echo "=== GraphQL review threads (resolved/unresolved, comments) ==="
gh api graphql -f owner=notedwin-dev -f name=finance-tracker-app -F number="$PR" -f query='
query($owner:String!, $name:String!, $number:Int!) {
  repository(owner:$owner, name:$name) {
    pullRequest(number:$number) {
      reviewThreads(first:100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first:20) {
            nodes {
              id
              author { login }
              body
              url
              createdAt
            }
          }
        }
      }
    }
  }
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select([.comments.nodes[].author.login | ascii_downcase | test("coderabbit")] | any) | {
  threadId:.id,
  isResolved,
  isOutdated,
  path,
  line,
  originalLine,
  comments:[.comments.nodes[] | {author:.author.login, url, createdAt, body}]
}' || true

Length of output: 276179

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="notedwin-dev/finance-tracker-app"
PR=10

echo "=== PR basic info ==="
gh pr view "$PR" --repo "$REPO" --json number,title,state,mergeable,reviewDecision,url,comments,reviews,files --jq '{
  number,title,state,mergeable,reviewDecision,url,
  commentsCount:(.comments|length),
  reviewsCount:(.reviews|length),
  filesCount:(.files|length)
}'

echo
echo "=== Review comments by coderabbitai / CodeRabbit-like bots ==="
gh api "repos/$REPO/pulls/$PR/comments?per_page=100" --paginate \
  --jq '.[] | select((.user.login|ascii_downcase) | test("coderabbit")) | {
    id,
    user:.user.login,
    path,
    line,
    original_line,
    position,
    in_reply_to_id,
    outdated,
    url:.html_url,
    body
  }' || true

echo
echo "=== Top-level issue comments by coderabbitai / CodeRabbit-like bots ==="
gh api "repos/$REPO/issues/$PR/comments?per_page=100" --paginate \
  --jq '.[] | select((.user.login|ascii_downcase) | test("coderabbit")) | {
    id,
    user:.user.login,
    created_at,
    updated_at,
    url:.html_url,
    body
  }' || true

echo
echo "=== GraphQL review threads (resolved/unresolved, comments) ==="
gh api graphql -f owner=notedwin-dev -f name=finance-tracker-app -F number="$PR" -f query='
query($owner:String!, $name:String!, $number:Int!) {
  repository(owner:$owner, name:$name) {
    pullRequest(number:$number) {
      reviewThreads(first:100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first:20) {
            nodes {
              id
              author { login }
              body
              url
              createdAt
            }
          }
        }
      }
    }
  }
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select([.comments.nodes[].author.login | ascii_downcase | test("coderabbit")] | any) | {
  threadId:.id,
  isResolved,
  isOutdated,
  path,
  line,
  originalLine,
  comments:[.comments.nodes[] | {author:.author.login, url, createdAt, body}]
}' || true

Length of output: 289231

---

---

@notedwin-dev The PR is still marked CHANGES_REQUESTED. From the review-thread output, these are the remaining unresolved items.

High priority / should fix before merge

1. src/lib/application/commands/accounts.ts

   • Issue: saveAccount can commit account changes before opening/adjustment transaction persistence succeeds.
   • Risk: Account balance and transaction ledger can diverge on partial failure.
   • Fix: Build transaction updates first, persist transactions first, then persist accounts and update stores only after both writes succeed.

2. src/lib/application/commands/transactions.ts

   • Issue: Batch transfer edits skip linked partner-leg updates when only shared fields change (amount, currency, date, fee, feeType, note/notes, shopName, etc.).
   • Risk: Linked transfer legs can become inconsistent.
   • Fix: Detect shared transfer field changes and include them in the condition that updates/recalculates the partner leg.

3. src/lib/domain/balance.engine.ts

   • Issue: computeBudgetConsumption and computeSavingsMovement still process historical transactions while account deltas skip them.
   • Risk: Pot/pocket balances can desync from accounts.
   • Fix: Early-return empty maps when t.isHistorical is true in both functions.

4. opencode.json

   • Issue: Subagents still have "bash": "allow".
   • Risk: Overly permissive shell execution for automated agents.
   • Fix: Replace blanket shell access with "bash": "deny" or a restricted/scoped policy.

5. components/AccountForm.tsx

   • Issue: sanitizeNote() PAN/card-number redaction regex is fragile.
   • Risk: Sensitive 13–19 digit values may be missed or over-redacted.
   • Fix: Match candidate digit/separator runs, strip non-digits, and redact only when the digit count is between 13 and 19. Keep CVV/CVC redaction.

Medium / architecture and correctness cleanup

6. src/lib/domain/balance.engine.ts

   • Issue: Imports normalizeDate from helpers, making a domain dependency implicit.
   • Fix: Inject a date normalizer as an explicit parameter or move normalization into a domain-local pure utility and update callers/tests.

7. src/lib/application/commands/sync.ts

   • Issue: loadData(profile, _forceUnlock) has unused parameters.
   • Fix: Remove unused params and update callers, or intentionally underscore both and document why they remain.

8. src/lib/application/commands/transactions.ts

   • Issue: Inline comments remain despite repo guidance preferring self-documenting names.
   • Fix: Remove comments or extract commented blocks into named helpers. This is more style/maintainability than functional.

9. src/lib/domain/migration.ts

   • Issue: Top-of-file temporary migration comment still in TypeScript module.
   • Fix: Move that temporary/removal note to ADR/docs and keep code self-documenting.

10. src/lib/domain/__tests__/migration.test.ts

    • Issue: Test title says “returns true” but assertion expects false.
    • Fix: Rename test title to match the assertion.

11. src/lib/application/commands/__tests__/sync.test.ts

    • Issue: Uses dynamic import for resetAndSync.
    • Fix: Import resetAndSync statically with syncData.

12. src/lib/application/commands/__tests__/sync.test.ts

    • Issue: vi.stubGlobal("confirm", ...) is not restored.
    • Fix: Add vi.unstubAllGlobals() in afterEach.

13. components/GoogleDrivePicker.tsx

    • Issue: Duplicate maskFileId implementation.
    • Fix: Extract one module-scope maskFileId(fileId: string): string.

14. src/lib/domain/__tests__/crypto.test.ts

    • Issue: Test is under domain tests but targets services/crypto.services.ts.
    • Fix: Move/co-locate it under a services test location and update imports.

Docs / markdown cleanup

15. docs/adrs/001-layered-architecture-refactor.md

    • Issue: ADR section order does not match template.
    • Fix: Reorder to Context → Decision → Consequences → Status.

16. docs/adrs/002-drop-vault-data-minimization.md

    • Issue: ADR section order does not match template.
    • Fix: Reorder to Context → Decision → Consequences → Status.

17. docs/PLAN.md

    • Issue: Fenced code blocks lack language tags.
    • Fix: Add text, typescript, json, etc.

18. PLAN.md

    • Issue: Fenced code blocks lack language tags.
    • Fix: Add text, typescript, json, etc.

19. PLAN.md

    • Issue: Stale usePrivacyStore / vault migration guidance remains.
    • Fix: Replace with current useMaskStore / mask-store guidance and remove vault/security-unlocked instructions.

20. layouts/MainLayout.tsx

    • Issue: Desktop Mask Mode toggle lacks toggle semantics.
    • Fix: Add type="button" and aria-pressed={maskMode}; optionally aria-label="Toggle mask mode".

---

Copy-paste fix prompt

Fix the remaining unresolved CodeRabbit review comments on PR `#10`.

Context:
- PR `#10` is mergeable but still has reviewDecision CHANGES_REQUESTED.
- Many CodeRabbit comments are already addressed; only fix still-valid unresolved items.
- Keep changes minimal and preserve the current layered architecture:
  - Domain = pure functions
  - Application commands = orchestration
  - Stores = Zustand state
  - No DataProvider/DataContext/vault backslides

Tasks:

1. Fix functional/data-integrity issues:
   - In src/lib/application/commands/accounts.ts, update saveAccount so opening/adjustment transactions are built and persisted before account changes are committed. Only update stores after transaction and account persistence succeeds.
   - In src/lib/application/commands/transactions.ts, ensure batch transfer edits update linked partner legs when shared fields change, including amount, currency, date, fee, feeType, note/notes, and shopName.
   - In src/lib/domain/balance.engine.ts, make computeBudgetConsumption and computeSavingsMovement return empty maps for historical transactions, matching account-delta behavior.
   - In components/AccountForm.tsx, harden sanitizeNote() PAN redaction by matching candidate digit/separator runs, stripping non-digits, and redacting only when the digit count is 13–19. Preserve CVV/CVC redaction.

2. Fix security/config issue:
   - In opencode.json, replace every "bash": "allow" for subagents with a safer policy, preferably "bash": "deny" unless a restricted allowlist is required.

3. Fix architecture/test cleanup:
   - In src/lib/domain/balance.engine.ts, remove hidden helper-layer dependency on normalizeDate. Make date normalization explicit via a passed normalizer or a domain-local pure utility, and update all callers/tests.
   - In src/lib/application/commands/sync.ts, remove or intentionally underscore unused loadData parameters and update callers.
   - In src/lib/application/commands/transactions.ts, remove inline comments or replace commented blocks with self-documenting helper names.
   - In src/lib/domain/migration.ts, move the temporary migration note out of the TypeScript module into docs/ADR context.
   - In src/lib/domain/__tests__/migration.test.ts, fix the mismatched test description so it says false when the assertion expects false.
   - In src/lib/application/commands/__tests__/sync.test.ts, statically import resetAndSync instead of dynamic import and restore vi.stubGlobal("confirm") using vi.unstubAllGlobals() in afterEach.
   - In components/GoogleDrivePicker.tsx, extract duplicate maskFileId implementations into one module-scope helper.
   - Move src/lib/domain/__tests__/crypto.test.ts to a services co-located test location because it tests services/crypto.services.ts, then update imports.

4. Fix docs/markdown:
   - Reorder docs/adrs/001-layered-architecture-refactor.md to Context → Decision → Consequences → Status.
   - Reorder docs/adrs/002-drop-vault-data-minimization.md to Context → Decision → Consequences → Status.
   - Add language tags to all fenced code blocks in docs/PLAN.md and PLAN.md.
   - In PLAN.md, replace stale usePrivacyStore/vault/securityUnlocked guidance with current useMaskStore/mask-store guidance.
   - In layouts/MainLayout.tsx, add type="button" and aria-pressed={maskMode} to desktop Mask Mode toggle buttons.

Validation:
- Run npx tsc --noEmit
- Run npm run test
- Re-check that no unresolved CodeRabbit comments remain except intentionally deferred low-value refactors, and explain any skipped item with a reason.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/application/commands/accounts.ts (1)

90-92: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block deletion when the account is referenced via toAccountId too.

Line 90 only checks t.accountId === accountId. Transfer rows also keep the destination account in toAccountId, so an account used only on that side can still be deleted and leave orphaned transactions.

Suggested fix

-  const referencing = existingTransactions.filter((t) => t.accountId === accountId);
+  const referencing = existingTransactions.filter(
+    (t) => t.accountId === accountId || t.toAccountId === accountId,
+  );

🤖 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/application/commands/accounts.ts` around lines 90 - 92, The deletion
guard only checks existingTransactions.filter(t => t.accountId === accountId) so
it misses transactions that reference the account via toAccountId; update the
check in the same function to filter for either t.accountId === accountId ||
t.toAccountId === accountId (or similar predicate) so referencing includes both
sides, update the referencing variable name if needed, and keep the existing
showToast message (or adjust wording) to reflect the combined count before
preventing deletion.

♻️ Duplicate comments (4)

components/AccountForm.tsx (1)

42-44: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Match whole digit runs before redacting.

This still redacts 13–19 digit substrings inside longer runs, so a 20-digit reference becomes [redacted]0. Match the full digit/separator run first, then redact only when the normalized digit count is 13–19.

Suggested fix

   const sanitizeNote = (value: string) =>
     value
       .replace(/\b(cvv|cvc)\s*:?\s*\d{3,4}\b/gi, "[redacted]")
-      .replace(/\d[ -]?(?:\d[ -]?){11,17}\d/g, (run) => {
+      .replace(/\d(?:[ -]?\d)+/g, (run) => {
         const digits = run.replace(/\D/g, "");
         return digits.length >= 13 && digits.length <= 19 ? "[redacted]" : run;
       });

🤖 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 `@components/AccountForm.tsx` around lines 42 - 44, The current .replace
callback in AccountForm.tsx is matching 13–19 digit substrings inside longer
runs; update the pattern used in the .replace call so it matches the entire
digit/separator run (the whole matched "run" passed into the callback) instead
of substrings, then keep the existing logic in the callback that computes digits
= run.replace(/\D/g, "") and only return "[redacted]" when digits.length is
between 13 and 19, otherwise return the original run; locate the .replace
invocation and the variables run and digits to implement this change.

src/lib/application/commands/accounts.ts (1)

63-72: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Persist the ledger entry before the account snapshot.

Line 65 still saves updated accounts before updatedTxs. If the transaction write fails or the process dies between those calls, the balance change lands without the compensating ACCOUNT_OPENING/ADJUSTMENT entry.

🤖 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/application/commands/accounts.ts` around lines 63 - 72, The current
sequence writes accounts (StorageService.saveAccounts(updated)) before
transactions, risking an account balance change without its ledger entry; change
the order to first persist the composed transaction list (await
StorageService.saveTransactions(updatedTxs)), then persist the account snapshot
(await StorageService.saveAccounts(updated)), and only after both succeed call
store.setTransactions(updatedTxs) and store.setAccounts(updated); if
saveTransactions fails, ensure you still restore the original accounts (await
StorageService.saveAccounts(existingAccounts)) and rethrow the error (as the
current catch already does), using the same symbols updatedTxs, updated,
existingAccounts, StorageService.saveTransactions, StorageService.saveAccounts,
store.setTransactions, and store.setAccounts to locate the changes.

docs/adrs/002-drop-vault-data-minimization.md (1)

16-54: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep the ADR’s top-level headings to the required template.

Considered Options and References still break the mandated Context → Decision → Consequences → Status order. Fold them into subsections under Context/Consequences so the ADR stays parseable and consistent.

Based on learnings, significant architectural decisions in docs/adrs/ should use the template Context → Decision → Consequences → Status.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/adrs/002-drop-vault-data-minimization.md` around lines 16 - 54, Move the
ADR headings to match the required template by folding the "Considered Options"
and "References" sections into the appropriate top-level template sections:
merge the "Considered Options" content under the "Context" section (as a
"Considered Options" subsection) and move the "References" list under
"Consequences" (as a "References" subsection), ensure the file now follows the
exact top-level order "Context → Decision → Consequences → Status", and keep the
subsection titles "Considered Options" and "References" intact for clarity.

src/lib/application/commands/transactions.ts (1)

536-545: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Partner-leg updates still skip shared-field-only transfer edits.

Line 536’s condition still excludes cases where only shared transfer fields change, so linked legs can remain out-of-sync even though Line 561 now maps note.

Suggested fix

   ids.forEach((id) => {
     const originalTx = txMap.get(id);
     if (!originalTx) return;

     const thisUpdates: any = { ...cleanUpdates };
+    const sharedTransferFieldChanged =
+      cleanUpdates.amount !== undefined ||
+      cleanUpdates.currency !== undefined ||
+      cleanUpdates.date !== undefined ||
+      cleanUpdates.fee !== undefined ||
+      cleanUpdates.feeType !== undefined ||
+      cleanUpdates.note !== undefined ||
+      cleanUpdates.shopName !== undefined;
@@
     if (
       originalTx.linkedTransactionId &&
       (nullifiedFields.includes("potId") ||
         nullifiedFields.includes("savingPocketId") ||
         nullifiedFields.includes("toSavingPocketId") ||
         cleanUpdates.accountId !== undefined ||
         cleanUpdates.toAccountId !== undefined ||
         cleanUpdates.savingPocketId !== undefined ||
-        cleanUpdates.toSavingPocketId !== undefined)
+        cleanUpdates.toSavingPocketId !== undefined ||
+        sharedTransferFieldChanged)
     ) {

Also applies to: 561-565

🤖 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/application/commands/transactions.ts` around lines 536 - 545, The
current guard around originalTx.linkedTransactionId skips partner-leg updates
when only shared transfer fields changed; update the condition in
transactions.ts so it also checks for changes to shared transfer fields (e.g.,
note and any other shared transfer fields your domain uses) by adding a
sharedTransferFields array and testing if any of those fields appear in
nullifiedFields or in Object.keys(cleanUpdates) — if any are present, do not
short-circuit the linked-leg update (this ensures the mapping at where note is
handled around the existing note mapping code still propagates).

🤖 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.

Outside diff comments:
In `@src/lib/application/commands/accounts.ts`:
- Around line 90-92: The deletion guard only checks
existingTransactions.filter(t => t.accountId === accountId) so it misses
transactions that reference the account via toAccountId; update the check in the
same function to filter for either t.accountId === accountId || t.toAccountId
=== accountId (or similar predicate) so referencing includes both sides, update
the referencing variable name if needed, and keep the existing showToast message
(or adjust wording) to reflect the combined count before preventing deletion.

---

Duplicate comments:
In `@components/AccountForm.tsx`:
- Around line 42-44: The current .replace callback in AccountForm.tsx is
matching 13–19 digit substrings inside longer runs; update the pattern used in
the .replace call so it matches the entire digit/separator run (the whole
matched "run" passed into the callback) instead of substrings, then keep the
existing logic in the callback that computes digits = run.replace(/\D/g, "") and
only return "[redacted]" when digits.length is between 13 and 19, otherwise
return the original run; locate the .replace invocation and the variables run
and digits to implement this change.

In `@docs/adrs/002-drop-vault-data-minimization.md`:
- Around line 16-54: Move the ADR headings to match the required template by
folding the "Considered Options" and "References" sections into the appropriate
top-level template sections: merge the "Considered Options" content under the
"Context" section (as a "Considered Options" subsection) and move the
"References" list under "Consequences" (as a "References" subsection), ensure
the file now follows the exact top-level order "Context → Decision →
Consequences → Status", and keep the subsection titles "Considered Options" and
"References" intact for clarity.

In `@src/lib/application/commands/accounts.ts`:
- Around line 63-72: The current sequence writes accounts
(StorageService.saveAccounts(updated)) before transactions, risking an account
balance change without its ledger entry; change the order to first persist the
composed transaction list (await StorageService.saveTransactions(updatedTxs)),
then persist the account snapshot (await StorageService.saveAccounts(updated)),
and only after both succeed call store.setTransactions(updatedTxs) and
store.setAccounts(updated); if saveTransactions fails, ensure you still restore
the original accounts (await StorageService.saveAccounts(existingAccounts)) and
rethrow the error (as the current catch already does), using the same symbols
updatedTxs, updated, existingAccounts, StorageService.saveTransactions,
StorageService.saveAccounts, store.setTransactions, and store.setAccounts to
locate the changes.

In `@src/lib/application/commands/transactions.ts`:
- Around line 536-545: The current guard around originalTx.linkedTransactionId
skips partner-leg updates when only shared transfer fields changed; update the
condition in transactions.ts so it also checks for changes to shared transfer
fields (e.g., note and any other shared transfer fields your domain uses) by
adding a sharedTransferFields array and testing if any of those fields appear in
nullifiedFields or in Object.keys(cleanUpdates) — if any are present, do not
short-circuit the linked-leg update (this ensures the mapping at where note is
handled around the existing note mapping code still propagates).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: db816171-9e2e-4ac8-bc80-cd7e915a1c9a

📥 Commits

Reviewing files that changed from the base of the PR and between 3f81a18 and b85ea43.

📒 Files selected for processing (16)

• components/AccountForm.tsx
• components/GoogleDrivePicker.tsx
• docs/PLAN.md
• docs/adrs/001-layered-architecture-refactor.md
• docs/adrs/002-drop-vault-data-minimization.md
• layouts/MainLayout.tsx
• opencode.json
• services/__tests__/crypto.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/accounts.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/transactions.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/balance.engine.ts
• src/lib/domain/dates.ts
• vite.config.ts

💤 Files with no reviewable changes (1)

• src/lib/application/commands/migration.ts

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

src/lib/domain/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Domain layer functions must be pure — no React, no side effects, no imports from services/ or stores/. All dependencies must be explicit parameters.

Use TransactionType enum from types for type discrimination in domain layer functions

Files:

• src/lib/domain/dates.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/balance.engine.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid comments — prefer self-documenting code

Files:

• src/lib/domain/dates.ts
• components/GoogleDrivePicker.tsx
• src/lib/domain/__tests__/migration.test.ts
• services/__tests__/crypto.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/accounts.ts
• vite.config.ts
• layouts/MainLayout.tsx
• src/lib/domain/balance.engine.ts
• components/AccountForm.tsx
• src/lib/application/commands/transactions.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

State lives in Zustand stores under src/stores/ — React components and domain functions must not manage local state that duplicates store state

Files:

• src/lib/domain/dates.ts
• components/GoogleDrivePicker.tsx
• src/lib/domain/__tests__/migration.test.ts
• services/__tests__/crypto.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/accounts.ts
• vite.config.ts
• layouts/MainLayout.tsx
• src/lib/domain/balance.engine.ts
• components/AccountForm.tsx
• src/lib/application/commands/transactions.ts

src/lib/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use convertAmount() function from src/lib/domain/currency.ts for MYR/USD currency conversion

Use computeAccountTransactionAmount() function from src/lib/domain/balance.engine.ts to calculate account balance impact per transaction

Use computeBudgetConsumption() function from src/lib/domain/balance.engine.ts to calculate budget consumed/replenished per transaction

Use computeSavingsMovement() function from src/lib/domain/balance.engine.ts to calculate savings added/withdrawn per transaction

Use matchesSearch() function from src/lib/domain/search.ts for transaction text matching operations

Files:

• src/lib/domain/dates.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/accounts.ts
• src/lib/domain/balance.engine.ts
• src/lib/application/commands/transactions.ts

docs/adrs/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Name ADR files sequentially as NNN-title-kebab-case.md in docs/adrs/ directory

Files:

• docs/adrs/002-drop-vault-data-minimization.md
• docs/adrs/001-layered-architecture-refactor.md

components/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Tailwind CSS utility-first styling for all UI components

React components must never call storage or sheet services directly — they must use command handlers in src/lib/application/commands/

Files:

• components/GoogleDrivePicker.tsx
• components/AccountForm.tsx

src/lib/domain/__tests__/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Co-locate unit tests for domain functions in __tests__/ subdirectories within src/lib/domain/

Write domain functions as pure unit tests using Vitest without jsdom

Files:

• src/lib/domain/__tests__/migration.test.ts

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Create feature branches with prefix `feature/*` for new features (e.g., `feature/export-csv`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Create refactor branches with prefix `refactor/*` for major code refactoring or UI revamp (e.g., `refactor/layered-architecture`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Create bugfix branches with prefix `bugfix/*` for bug fixes (e.g., `bugfix/search-overlay-crash`)

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Follow the PR workflow: create feature/bugfix/refactor branches from `main`, develop on the branch, push and open PR to `staging`, review on `staging`, then merge `staging` into `main` for auto-deployment to Vercel

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Document significant architectural decisions in Architecture Decision Records (ADRs) located in `docs/adrs/` using the template: Context → Decision → Consequences → Status

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Run all tests with `npm run test`, watch mode with `npm run test:watch`

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T19:32:15.434Z
Learning: Read the latest ADR in `docs/adrs/` before making architectural changes to understand current decisions and constraints

🔇 Additional comments (7)

docs/PLAN.md (1)

7-7: LGTM!

Also applies to: 36-36, 53-53, 82-82, 90-90, 102-102, 134-134, 147-147, 149-150, 162-162

docs/adrs/001-layered-architecture-refactor.md (1)

268-272: LGTM!

services/__tests__/crypto.test.ts (1)

7-7: LGTM!

src/lib/application/commands/__tests__/sync.test.ts (1)

5-5: LGTM!

Also applies to: 97-97

src/lib/domain/balance.engine.ts (1)

2-2: LGTM!

Also applies to: 12-12, 45-45

src/lib/domain/dates.ts (1)

1-26: LGTM!

vite.config.ts (1)

68-68: LGTM!

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/lib/application/commands/accounts.ts (1)

65-70: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Rollback is incomplete on partial failure and can still desync ledger/account state.

If Line 66 succeeds and Line 67 fails, Line 69 restores only accounts; newly persisted transactions are left behind. This still allows divergence under partial failure.

Suggested patch

   if (newTxs.length > 0) {
     const updatedTxs = [...newTxs, ...existingTransactions];
     try {
       await StorageService.saveTransactions(updatedTxs);
       await StorageService.saveAccounts(updated);
     } catch (err) {
-      await StorageService.saveAccounts(existingAccounts);
+      await Promise.allSettled([
+        StorageService.saveTransactions(existingTransactions),
+        StorageService.saveAccounts(existingAccounts),
+      ]);
       throw err;
     }
     store.setTransactions(updatedTxs);
     store.setAccounts(updated);

🤖 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/application/commands/accounts.ts` around lines 65 - 70, The current
try/catch around StorageService.saveTransactions(updatedTxs) and
StorageService.saveAccounts(updated) only restores accounts on error, leaving
newly saved transactions if saveAccounts fails; update the error path to fully
rollback transactions as well so ledger/account state cannot desync.
Specifically, inside the catch for the block containing saveTransactions and
saveAccounts, after calling StorageService.saveAccounts(existingAccounts) also
remove or restore transaction state to its previous value (e.g., call
StorageService.deleteTransactions(updatedTxs) or
StorageService.saveTransactions(existingTransactions) depending on available
APIs), or preferably use an atomic/storage transaction API if StorageService
supports it; ensure this change references saveTransactions, saveAccounts,
updatedTxs and existingAccounts so both sides are consistently rolled back on
failure.

🤖 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 `@docs/adrs/002-drop-vault-data-minimization.md`:
- Around line 36-42: The markdown headings "Migration shape" and "References" in
docs/adrs/002-drop-vault-data-minimization.md need a blank line after each to
satisfy MD022; edit that file to insert a single empty line immediately after
those two headings so the following paragraph/blockquote list is separated,
leaving the rest of content (including mentions of migrateSchemaV1toV2,
ProfilePage.handleExportData, and src/lib/application/commands/privacy.ts)
unchanged.

---

Duplicate comments:
In `@src/lib/application/commands/accounts.ts`:
- Around line 65-70: The current try/catch around
StorageService.saveTransactions(updatedTxs) and
StorageService.saveAccounts(updated) only restores accounts on error, leaving
newly saved transactions if saveAccounts fails; update the error path to fully
rollback transactions as well so ledger/account state cannot desync.
Specifically, inside the catch for the block containing saveTransactions and
saveAccounts, after calling StorageService.saveAccounts(existingAccounts) also
remove or restore transaction state to its previous value (e.g., call
StorageService.deleteTransactions(updatedTxs) or
StorageService.saveTransactions(existingTransactions) depending on available
APIs), or preferably use an atomic/storage transaction API if StorageService
supports it; ensure this change references saveTransactions, saveAccounts,
updatedTxs and existingAccounts so both sides are consistently rolled back on
failure.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3673fc75-e6f0-446e-85be-a2da61b8b1bc

📥 Commits

Reviewing files that changed from the base of the PR and between b85ea43 and cc49a9e.

📒 Files selected for processing (4)

• components/AccountForm.tsx
• docs/adrs/002-drop-vault-data-minimization.md
• src/lib/application/commands/accounts.ts
• src/lib/application/commands/transactions.ts

📜 Review details

🧰 Additional context used

📓 Path-based instructions (5)

docs/adrs/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Create Architecture Decision Records (ADRs) in docs/adrs/ with sequential numbering (NNN-title-kebab-case.md) following the template: Context → Decision → Consequences → Status

Files:

• docs/adrs/002-drop-vault-data-minimization.md

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Tailwind CSS utility-first styling throughout the application

Avoid comments in code; prefer self-documenting code with clear naming

Files:

• src/lib/application/commands/accounts.ts
• components/AccountForm.tsx
• src/lib/application/commands/transactions.ts

src/lib/{domain,application}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TransactionType enum from types for type discrimination in domain functions

Files:

• src/lib/application/commands/accounts.ts
• src/lib/application/commands/transactions.ts

src/lib/application/commands/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Command handlers in src/lib/application/commands/ must orchestrate domain logic and state management operations

Files:

• src/lib/application/commands/accounts.ts
• src/lib/application/commands/transactions.ts

{components,pages,layouts}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

React component files must not call storage or sheet services directly; all external operations must go through command handlers in src/lib/application/commands/

Files:

• components/AccountForm.tsx

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T20:32:28.594Z
Learning: Use feature/*, refactor/*, bugfix/*, staging, and main git branch prefixes according to ZenFinance conventions

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T20:32:28.594Z
Learning: Run tests with npm run test for single run or npm run test:watch for watch mode; test files are co-located in __tests__/ directories

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T20:32:28.594Z
Learning: Significant architectural decisions must be documented in ADRs before implementation; read the latest ADR before making architectural changes

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T20:32:28.594Z
Learning: Use domain layer functions for currency conversion, balance computation, budget tracking, savings movement, and transaction matching as defined in the Key Domain Functions table

🪛 markdownlint-cli2 (0.22.1)

docs/adrs/002-drop-vault-data-minimization.md

[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🔇 Additional comments (2)

components/AccountForm.tsx (1)

39-45: LGTM!

src/lib/application/commands/transactions.ts (1)

521-548: LGTM!

Also applies to: 565-569

[notedwin-dev]

@coderabbitai do final full code review

[coderabbitai]

@notedwin-dev Sure! I'll kick off a full re-review of PR #10 now, covering all the changes across the layered architecture refactor, vault removal (ADR-002), and the Sheets-401 / toast fix. I'll re-examine everything including the previously flagged high-priority items (transaction ordering in saveAccount, partner-leg sync in batch edits, historical transaction handling in computeBudgetConsumption/computeSavingsMovement, opencode.json bash permissions, and sanitizeNote hardening) as well as the medium and docs items.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 27

♻️ Duplicate comments (1)

PLAN.md (1)

145-157: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Replace stale vault/privacy references with mask-store equivalents.

Lines 145 and 157 still reference usePrivacyStore, setVaultUnlocked(), isVaultEnabled, and isVaultUnlocked, which conflict with the vault removal (ADR-002) in this PR. Replace these with the current useMaskStore APIs to prevent confusion and backsliding.

🤖 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 `@PLAN.md` around lines 145 - 157, Replace all lingering usePrivacyStore
references and vault method names with the mask-store equivalents: change calls
like usePrivacyStore.getState().setVaultUnlocked() inside
setSecurityUnlockedWithRef to the corresponding useMaskStore API (e.g.,
useMaskStore.getState().setMaskUnlocked() or the project’s exact setter), and
update reads of isVaultEnabled/isVaultUnlocked in pages
(DashboardPage/AssetsPage/AccountPage) to use useMaskStore’s boolean selectors
(e.g., useMaskStore().isMaskEnabled / useMaskStore().isMaskUnlocked or the
store’s exact property names). Ensure all imports of usePrivacyStore are
replaced with useMaskStore and method/property names are updated consistently.

🤖 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 `@AGENTS.md`:
- Around line 3-4: Add required blank lines around top-level headings and fenced
code blocks and include a language identifier for fenced blocks (for example
change ``` to ```text) to satisfy MD022/MD031/MD040; specifically, ensure there
is an empty line above and below "## Project Structure" and every fenced block,
update the fences to include a language token (e.g., text), and apply the same
fixes to the other affected heading/fence occurrences noted (around lines
referenced in the review) so all headings and code fences have proper
surrounding blank lines and a fenced language.

In `@App.tsx`:
- Around line 51-57: Remove the inline JSX comments like {/* Public Routes */}
and {/* Protected App Routes */} in App.tsx; instead make the grouping
self-descriptive by either extracting the sets of Route elements into named
components (e.g., PublicRoutes and ProtectedRoutes) or using clearly named JSX
wrappers/fragments so the Route entries (LandingPage, AuthPage, PrivacyPolicy,
TermsOfService, etc.) are organized without comments; ensure any other similar
inline comment nodes (including the one near Routes around the protected
section) are deleted and replaced with the descriptive structure.

In `@components/AccountForm.tsx`:
- Line 12: The onSave prop on the AccountForm component is typed as returning
void but is awaited where it's used; update the prop signature to return a
Promise (e.g., onSave: (account: Omit<Account, "userId">) => Promise<void>) and
adjust any related callers to return/propagate a Promise as needed so awaiting
the call is type-safe; look for the AccountForm props/interface and the function
using await onSave to ensure both sides match.

In `@components/Profile.tsx`:
- Around line 43-44: The "Recalculate Balances" action is rendered as an
always-clickable setting even though the prop onRecalculateBalances is optional;
update the Profile component to guard rendering and interaction by checking
onRecalculateBalances before enabling the control: either hide the control when
onRecalculateBalances is undefined or render it in a disabled state (and prevent
clicks) and optionally show a tooltip/message; ensure the click handler bound in
the Recalculate Balances setting uses the presence check (e.g., if
(!onRecalculateBalances) return) and the UI reflects disabled state so there is
no silent no-op when onRecalculateBalances is not provided.

In `@docs/adrs/001-layered-architecture-refactor.md`:
- Around line 120-128: The code block showing the command flow for
handleTransactionSubmit is missing a language identifier; update the opening
fence for that snippet to include a language tag (e.g., change ``` to ```text or
```typescript) so the example is properly labeled—locate the code block
containing handleTransactionSubmit and add the appropriate language identifier
to the opening ``` fence.
- Around line 32-56: The fenced code block containing the ASCII architecture
diagram is missing a language identifier, so update the Markdown fenced block
around the diagram (the triple-backtick block in the ADR content) to include the
language tag "text" (i.e., replace ``` with ```text) so linters and renderers
treat it as plain text; locate the diagram block in the ADR (the big box
starting with "┌─────────────────────────────────────────────┐") and add the tag
to the opening fence accordingly.

In `@docs/issues/001-rename-and-split-computeBalanceDeltas.md`:
- Around line 45-47: The migration plan is contradictory about
computeAccountTransactionAmount — it is introduced as the new API earlier but
also listed for deletion; fix by choosing one intent and making the doc
consistent: either keep computeAccountTransactionAmount as the new exported API
and remove the "Delete `computeAccountTransactionAmount`" bullet (and instead
describe how to deprecate old APIs like computeBalanceDeltas and
AccountTransactionMap), or if the intent is to remove it, change earlier lines
that introduce computeAccountTransactionAmount to instead introduce the three
split functions and update the call-site change notes for commands.ts and
DataProvider.tsx accordingly so computeAccountTransactionAmount and
AccountTransactionMap are no longer referenced. Ensure the doc references the
exact symbols computeAccountTransactionAmount, AccountTransactionMap,
computeBalanceDeltas, commands.ts, and DataProvider.tsx so readers can find and
apply the changes.

In `@docs/issues/003-privacy-store.md`:
- Around line 8-9: The document still references the old store API names:
replace occurrences of privacyMode and setPrivacyMode with the current mask
store names maskMode and setMaskMode, and update any mentions of
DataProvider-held state (maskAmount, maskText, privacyMode) to reflect that mask
state now lives in mask.store.ts (maskAmount, maskText, maskMode); ensure the
text and examples use maskMode/setMaskMode and reference mask.store.ts instead
of privacy.store.ts or DataProvider so the issue plan matches the current
contract.

In `@helpers/useAppInit.tsx`:
- Around line 29-33: The code currently allows syncData to run before
loadData(profile).then(runVaultSchemaMigration()) completes; prevent auto-sync
by keeping the "not synced" gate until both loadData and runVaultSchemaMigration
finish: call setHasSynced(false) before starting, chain
loadData(profile).then(() => runVaultSchemaMigration()) and only after that
promise resolves setHasSynced(true) (and propagate errors in the catch to keep
the gate closed); update the logic around the sync trigger (where syncData is
invoked) to check hasSynced so syncData only runs after the resolved chain
completes, referencing setHasSynced, loadData, runVaultSchemaMigration, and
syncData.

In `@layouts/MainLayout.tsx`:
- Line 226: Remove the inline JSX comment {/* Mask Mode Toggle */} in
MainLayout.tsx and replace its intent with a self-documenting element name or
attribute: either rename the surrounding component/element to a descriptive name
(e.g., MaskModeToggle or ToggleMaskMode) or add a clear aria-label/title on the
existing JSX element that currently sits where the comment is, so the UI intent
is conveyed without an inline comment.

In `@pages/AccountPage.tsx`:
- Line 12: AccountPage.tsx is directly calling SheetService.isClientReady() (and
nearby sheet readiness logic lines ~676-690); move this logic into an
application command instead and have the page call that command. Create or reuse
a command in src/lib/application/commands (e.g., a checkSheetClientReadyCommand
or add to an existing sheet-related command handler) that performs the
SheetService.isClientReady() call and returns the minimal page-level result
(ready flag / error). Replace direct calls to SheetService.isClientReady() in
AccountPage.tsx with an invocation of the new command, passing only page inputs
and using the command's result to drive UI state.

In `@pages/ProfilePage.tsx`:
- Around line 85-96: The page component calls SheetService.isClientReady()
inside handleRecalculateBalances which violates the rule that pages must not
call storage/sheet services directly; instead create (or update) an application
command e.g. recalculateBalancesCommand in src/lib/application/commands that
accepts accounts, pots, pockets, transactions, usdRate, profileId and resolves
isCloud by calling SheetService.isClientReady() inside the command (or have the
store provide the ready flag), then have ProfilePage.handleRecalculateBalances
call recalculateBalancesCommand (or dispatch the command) and pass the data but
not call SheetService.isClientReady() itself; update any references to
recalculateBalances to use the new command API and remove direct SheetService
usage from the page.

In `@services/__tests__/crypto.test.ts`:
- Around line 63-70: Remove the inline comments and make the test
self-documenting by replacing the commented steps with clear, intent-revealing
identifiers and/or small helper functions: e.g., use a named helper
decodeBase64ToBytes(parts[3]) to get hashBytes, call flipFirstByte(hashBytes) to
mutate, and encodeBytesToBase64(hashBytes) to produce tamperedHash (or rename
variables to decodeHashBytes and tamperedHashBytes and use a single-line
re-encode expression). Update the test to call these helpers or use expressive
variable names so the three operations (decode, flip byte, re-encode) are
explicit without comments; keep the same logic and variable references (parts,
hashBytes, tamperedHash).

In `@services/sheets.services.ts`:
- Around line 29-31: Remove the inline/doc comment and make the intent
self-descriptive by renaming the symbol and/or using a helper: replace the
comment + ambiguous identifier with a clearly named function like
maskFileIdForLogging (or maskedFileIdForLog) and move any masking logic into
that helper, then update all usages (including the other instances flagged) to
call maskFileIdForLogging so the code documents itself without comments.
- Around line 94-99: The init flow sets gapiInitializing = true but never resets
it on early returns or errors; update the initialization function so every
early-exit branch (e.g., the getApiKey() missing branch that calls logger.warn,
and any other returns or catches in the same init routine) sets gapiInitializing
= false before returning, or better wrap the whole init sequence in a
try/finally and set gapiInitializing = false in the finally block to guarantee
reset on success, error, or early return; apply this change to the getApiKey()
branch and the other init/error branches where gapiInitializing is currently
left true.

In `@src/lib/application/commands/__tests__/sync.test.ts`:
- Around line 104-135: The test contains inline narrative comments (e.g., before
localStorage setup, assertions checking preserved keys, and removed keys) that
should be removed and replaced with clearer test structure; update the test
around resetAndSync, useSyncStore.getState, and the localStorage assertions by
deleting the inline comments and instead rename the test or add nested
describe/it blocks (or clearer assertion grouping) so expectations about
"keep-list keys preserved" and "non-kept keys removed" are self-explanatory
without inline comments; ensure StorageService.KEYS.PROFILE, the resetAndSync
call, and the loginWithGoogle/ updateProfile mocks remain unchanged while
removing the comment lines at the positions flagged.

In `@src/lib/application/commands/balance.ts`:
- Around line 72-105: The date-filtered branch computes
updatedAccounts/updatedPots/updatedPockets but never persists or applies them;
update the logic so that after computing those arrays you call
StorageService.saveAccounts(updatedAccounts)/savePots/savePockets and call
store.setAccounts(updatedAccounts)/setPots/setPockets for the filtered run as
well (i.e., move or duplicate the save+set calls so they run when startDate or
endDate are present), then run the existing cloud updates under isCloudEnabled
as appropriate (or keep them limited to the no-date case if intended), and only
call showToast("Recalculation complete", "success") after persistence/store
updates complete; reference updatedAccounts/updatedPots/updatedPockets,
StorageService.saveAccounts/savePots/savePockets,
store.setAccounts/setPots/setPockets, isCloudEnabled, SheetService.updateMany,
and showToast.

In `@src/lib/application/commands/categories.ts`:
- Around line 24-25: The code currently calls store.setCategories(updated)
before awaiting StorageService.saveCategories(updated), which can leave
in-memory Zustand state inconsistent if the save fails; change the flow in the
functions using store.setCategories and the second occurrence (lines showing
store.setCategories / StorageService.saveCategories) to await
StorageService.saveCategories(updated) first, only call
store.setCategories(updated) after the save succeeds, and handle/save errors
(log and return or surface the error) so you do not mutate the store on
persistence failure.

In `@src/lib/application/commands/goals.ts`:
- Around line 23-25: Persist changes to durable storage before mutating
in-memory Zustand state: in the functions that call store.setGoals(updated)
(e.g., the create/update goal handlers where you call
StorageService.saveGoals(updated) and showToast(...)), swap the order so you
await StorageService.saveGoals(updated) first and only then call
store.setGoals(updated) and showToast(...); also handle save errors by catching
and showing an error toast (and do not call store.setGoals on failure) so the UI
remains consistent with durable storage.

In `@src/lib/application/commands/helpers.ts`:
- Around line 1-3: The current generateId function (and idCounter) uses
Date.now() + Math.random() which risks collisions and diverges from the project
standard; replace it with a UUID-based ID using crypto.randomUUID() (e.g.,
return `tx_${crypto.randomUUID()}`) and remove the idCounter variable, ensuring
generateId() uses the built-in crypto API to produce collision-resistant IDs
consistent with other code that uses UUIDs.

In `@src/lib/application/commands/pockets.ts`:
- Around line 37-47: When deleting a pocket, persist the transaction cleanup
before persisting the pocket to avoid dangling references: move the
updatedTransactions creation and call to
StorageService.saveTransactions(updatedTransactions) (and then
store.setTransactions(updatedTransactions)) to occur before calling
StorageService.savePockets(updatedPockets) and store.setPockets(updatedPockets).
Update the code around the existingTransactions mapping and the calls to
StorageService.saveTransactions, StorageService.savePockets,
store.setTransactions, and store.setPockets so transactions are saved first,
then pockets are saved, and in-memory store updates follow the same safe order.

In `@src/lib/application/commands/sync.ts`:
- Line 112: Remove the inline comment "// Module-level refs for sync debouncing
(replaces React useRef)" and rely on clear, self-documenting variable/function
names instead; locate that exact comment in the top of the module (the one
referencing module-level refs and React useRef) and delete it so the codebase
follows the guideline of preferring self-explanatory identifiers over inline
comments.
- Around line 144-278: processSubscriptions mixes orchestration with business
rules (date advancement/recurrence, transaction id creation, dedupe semantics,
and currency conversion); extract pure domain functions and replace inline logic
with calls: create a pure computeNextOccurrences(sub, today, maxIterations) that
returns { nextDateStr, generatedTxDates[], bailed } (used where parseDateSafe,
nextDateStr and the while loop are), a pure buildSubscriptionTransaction(sub,
date, userId) that builds the Transaction object (used where newTx is created
and id/shopName/date are set), a pure
convertTransactionAmountForAccount(transaction, account, usdRate, rateValid)
that returns numeric amount (used where accUpdates computes amount and handles
USD/MYR conversion and rateValid checks), and a pure
dedupeTransactions(existingTxs, candidateTxs) that returns the deduped list
(used where newTxs are filtered). Keep processSubscriptions only orchestrating:
call these domain functions, update accUpdates/updatedSubs/updatedAccounts, and
persist/showToast as before.
- Around line 226-232: When dedupedNewTxs is empty the code returns early and
never persists the advanced nextPaymentDate values held in updatedSubs; ensure
updatedSubs is applied even when dedupedNewTxs.length === 0 by invoking the same
persistence/update routine used for non-empty cases (i.e., call the function
that saves/applies subscription updates for updatedSubs) before returning, then
show the no-new-transactions toast via useSyncStore.getState().showToast as
currently done.

In `@src/lib/application/commands/transactions.ts`:
- Around line 145-153: The code currently mutates the in-memory store via
store.setTransactions / store.setAccounts / store.setPots / store.setPockets and
then fires StorageService.saveTransactions / saveAccounts / savePots /
savePockets without awaiting, which can expose UI/state before persistence
succeeds; change the flow so you first call and await the corresponding
StorageService.save* calls, check for success (handle or surface errors), and
only after successful persistence perform the store.setTransactions /
store.setAccounts / store.setPots / store.setPockets mutations (apply the same
fix for the other identical blocks that call StorageService.save* and store.set*
in this file).
- Around line 44-72: This command embeds domain rules (cross-currency check and
delta-computation orchestration) inside application logic: extract the business
logic into domain functions/services and call them from the command;
specifically move the isCrossCurrency logic and the usdRate/cross-currency
validation into a domain/validation function (e.g.,
TransactionDomain.validateExchangeRate(tx, txAccount, toAccount, usdRate)) and
move the applyDeltas orchestration into a domain service (e.g.,
TransactionDomain.computeDeltas(tx, factor, accounts, pots, pockets) that
internally uses computeAccountTransactionAmount, computeBudgetConsumption,
computeSavingsMovement) and then replace the inline isCrossCurrency/if-check and
applyDeltas usage in this file with calls to those domain functions (keeping
showToast usage only for presenting errors returned by the domain validation).

In `@src/lib/domain/__tests__/balance.engine.test.ts`:
- Around line 219-264: Add tests mirroring the existing isHistorical assertion
for computeAccountTransactionAmount: create test cases for
computeBudgetConsumption and computeSavingsMovement that pass transactions with
isHistorical: true (using makeTx) and assert the result flips sign (i.e.,
consumption/movement is multiplied by -1) compared to the non-historical case;
also include a historical case for when no potId and when transaction is before
pot reset date to ensure size is 0/undefined as appropriate. Reference
computeBudgetConsumption, computeSavingsMovement, makeTx, makePot and
TransactionType to locate where to add these assertions.

---

Duplicate comments:
In `@PLAN.md`:
- Around line 145-157: Replace all lingering usePrivacyStore references and
vault method names with the mask-store equivalents: change calls like
usePrivacyStore.getState().setVaultUnlocked() inside setSecurityUnlockedWithRef
to the corresponding useMaskStore API (e.g.,
useMaskStore.getState().setMaskUnlocked() or the project’s exact setter), and
update reads of isVaultEnabled/isVaultUnlocked in pages
(DashboardPage/AssetsPage/AccountPage) to use useMaskStore’s boolean selectors
(e.g., useMaskStore().isMaskEnabled / useMaskStore().isMaskUnlocked or the
store’s exact property names). Ensure all imports of usePrivacyStore are
replaced with useMaskStore and method/property names are updated consistently.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ee72e72e-4374-4d89-a36f-89854db6b0ef

📥 Commits

Reviewing files that changed from the base of the PR and between 0d84549 and b1c5955.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (99)

• .gitignore
• .opencode/prompts/commands-splitter.txt
• .opencode/prompts/page-migrator.txt
• .opencode/prompts/state-migrator.txt
• AGENTS.md
• App.tsx
• CONTEXT.md
• PLAN.md
• components/AIInsights.tsx
• components/AccountCard.tsx
• components/AccountForm.tsx
• components/BulkImportModal.tsx
• components/Charts.tsx
• components/Goals.tsx
• components/GoogleDrivePicker.tsx
• components/History.tsx
• components/Profile.tsx
• components/SubscriptionManager.tsx
• components/history/SearchOverlay.tsx
• components/history/useFilteredTransactions.ts
• context/DataContext.tsx
• context/DataProvider.tsx
• context/types.ts
• docs/PLAN.md
• docs/adrs/001-layered-architecture-refactor.md
• docs/adrs/002-drop-vault-data-minimization.md
• docs/agents/domain.md
• docs/agents/issue-tracker.md
• docs/agents/triage-labels.md
• docs/issues/001-rename-and-split-computeBalanceDeltas.md
• docs/issues/003-privacy-store.md
• docs/issues/004-commands-extraction.md
• docs/issues/005-cleanup.md
• docs/issues/README.md
• docs/issues/review-pr2.md
• helpers/useAppInit.tsx
• helpers/useMask.tsx
• layouts/MainLayout.tsx
• opencode.json
• package.json
• pages/AccountPage.tsx
• pages/AssetsPage.tsx
• pages/DashboardPage.tsx
• pages/GoalsPage.tsx
• pages/HistoryPage.tsx
• pages/ProfilePage.tsx
• services/__tests__/crypto.test.ts
• services/auth.services.tsx
• services/biometric.services.ts
• services/coin.services.ts
• services/crypto.services.ts
• services/exchange.services.ts
• services/gemini.services.ts
• services/security.services.ts
• services/sheets.services.ts
• services/storage.services.ts
• services/twofa.services.ts
• skills-lock.json
• src/lib/application/__tests__/commands.test.ts
• src/lib/application/__tests__/logger.test.ts
• src/lib/application/commands.ts
• src/lib/application/commands/__tests__/migration.test.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/accounts.ts
• src/lib/application/commands/balance.ts
• src/lib/application/commands/categories.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/goals.ts
• src/lib/application/commands/helpers.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/commands/pots.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/application/commands/sync.ts
• src/lib/application/commands/transactions.ts
• src/lib/application/logger.ts
• src/lib/domain/__tests__/SearchOverlay.test.tsx
• src/lib/domain/__tests__/balance.engine.test.ts
• src/lib/domain/__tests__/currency.test.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/balance.engine.ts
• src/lib/domain/currency.ts
• src/lib/domain/dates.ts
• src/lib/domain/migration.ts
• src/lib/domain/search.ts
• src/stores/__tests__/finance.store.test.ts
• src/stores/__tests__/mask.store.test.ts
• src/stores/__tests__/sync.store.test.ts
• src/stores/__tests__/ui.store.test.ts
• src/stores/finance.store.ts
• src/stores/index.ts
• src/stores/mask.store.ts
• src/stores/sync.store.ts
• src/stores/ui.store.ts
• src/test-setup.ts
• tsconfig.json
• types.ts
• vite.config.ts

💤 Files with no reviewable changes (5)

• services/biometric.services.ts
• context/types.ts
• services/twofa.services.ts
• services/security.services.ts
• context/DataContext.tsx

📜 Review details

🧰 Additional context used

📓 Path-based instructions (13)

tsconfig.json

📄 CodeRabbit inference engine (AGENTS.md)

TypeScript configuration must be defined in tsconfig.json

Files:

• tsconfig.json

src/stores/__tests__/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Zustand stores: Store action tests using Vitest with jsdom

Files:

• src/stores/__tests__/ui.store.test.ts
• src/stores/__tests__/mask.store.test.ts
• src/stores/__tests__/sync.store.test.ts
• src/stores/__tests__/finance.store.test.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid comments — prefer self-documenting code

Files:

• src/stores/__tests__/ui.store.test.ts
• components/BulkImportModal.tsx
• src/lib/domain/currency.ts
• components/SubscriptionManager.tsx
• src/stores/mask.store.ts
• services/coin.services.ts
• components/AccountCard.tsx
• src/stores/index.ts
• components/Goals.tsx
• components/AIInsights.tsx
• components/history/SearchOverlay.tsx
• src/lib/domain/__tests__/currency.test.ts
• src/lib/application/commands/helpers.ts
• pages/DashboardPage.tsx
• pages/GoalsPage.tsx
• src/stores/__tests__/mask.store.test.ts
• src/test-setup.ts
• src/stores/__tests__/sync.store.test.ts
• services/exchange.services.ts
• pages/HistoryPage.tsx
• components/Charts.tsx
• src/stores/ui.store.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands.ts
• src/lib/application/logger.ts
• src/lib/application/commands/pockets.ts
• pages/AssetsPage.tsx
• src/lib/application/__tests__/logger.test.ts
• App.tsx
• services/__tests__/crypto.test.ts
• src/lib/domain/__tests__/balance.engine.test.ts
• src/lib/application/__tests__/commands.test.ts
• src/lib/application/commands/categories.ts
• src/lib/application/commands/migration.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/application/commands/goals.ts
• components/History.tsx
• services/gemini.services.ts
• helpers/useMask.tsx
• src/lib/application/commands/balance.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/domain/__tests__/SearchOverlay.test.tsx
• src/stores/sync.store.ts
• services/storage.services.ts
• helpers/useAppInit.tsx
• src/lib/application/commands/__tests__/migration.test.ts
• src/lib/domain/dates.ts
• src/stores/__tests__/finance.store.test.ts
• src/lib/application/commands/pots.ts
• src/lib/application/commands/chat-sessions.ts
• components/GoogleDrivePicker.tsx
• types.ts
• src/lib/domain/search.ts
• src/lib/application/commands/accounts.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• services/crypto.services.ts
• components/history/useFilteredTransactions.ts
• pages/AccountPage.tsx
• src/stores/finance.store.ts
• components/Profile.tsx
• services/auth.services.tsx
• pages/ProfilePage.tsx
• src/lib/application/commands/sync.ts
• components/AccountForm.tsx
• vite.config.ts
• src/lib/domain/migration.ts
• layouts/MainLayout.tsx
• src/lib/domain/balance.engine.ts
• src/lib/application/commands/transactions.ts
• services/sheets.services.ts

components/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

React components must not call storage or sheet services directly — they must go through command handlers in src/lib/application/commands/

State lives in Zustand stores under src/stores/ — components must read from stores instead of DataContext or direct service calls

Use Tailwind CSS for styling with utility-first approach

Files:

• components/BulkImportModal.tsx
• components/SubscriptionManager.tsx
• components/AccountCard.tsx
• components/Goals.tsx
• components/AIInsights.tsx
• components/history/SearchOverlay.tsx
• components/Charts.tsx
• components/History.tsx
• components/GoogleDrivePicker.tsx
• components/history/useFilteredTransactions.ts
• components/Profile.tsx
• components/AccountForm.tsx

src/lib/domain/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Domain layer functions must be pure functions with no React, no side effects, and no imports from services/ or stores/

All dependencies in domain layer functions must be explicit parameters

Domain layer modules must use one module per file with co-located tests in __tests__/ directories

Use TransactionType enum from types for type discrimination in domain functions

Files:

• src/lib/domain/currency.ts
• src/lib/domain/__tests__/currency.test.ts
• src/lib/domain/__tests__/balance.engine.test.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/dates.ts
• src/lib/domain/search.ts
• src/lib/domain/__tests__/matchesSearch.test.ts
• src/lib/domain/migration.ts
• src/lib/domain/balance.engine.ts

.gitignore

📄 CodeRabbit inference engine (AGENTS.md)

Do not track .env, .agents/, node_modules/, dist/, build/, .next/ in Git — add to .gitignore

Track docs/ in Git — ensure .gitignore rules are commented out for documentation directory

Files:

• .gitignore

src/lib/domain/__tests__/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Domain functions: Pure unit tests using Vitest without jsdom

Files:

• src/lib/domain/__tests__/currency.test.ts
• src/lib/domain/__tests__/balance.engine.test.ts
• src/lib/domain/__tests__/migration.test.ts
• src/lib/domain/__tests__/matchesSearch.test.ts

src/lib/application/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Application layer in src/lib/application/ must orchestrate commands without business logic

Files:

• src/lib/application/commands/helpers.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands.ts
• src/lib/application/logger.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/__tests__/logger.test.ts
• src/lib/application/__tests__/commands.test.ts
• src/lib/application/commands/categories.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/goals.ts
• src/lib/application/commands/balance.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/application/commands/__tests__/migration.test.ts
• src/lib/application/commands/pots.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/accounts.ts
• src/lib/application/commands/sync.ts
• src/lib/application/commands/transactions.ts

src/lib/application/commands/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Extract command handlers to src/lib/application/commands.ts or src/lib/application/commands/ instead of keeping them in component contexts

Files:

• src/lib/application/commands/helpers.ts
• src/lib/application/commands/__tests__/sync.test.ts
• src/lib/application/commands/pockets.ts
• src/lib/application/commands/categories.ts
• src/lib/application/commands/migration.ts
• src/lib/application/commands/goals.ts
• src/lib/application/commands/balance.ts
• src/lib/application/commands/subscriptions.ts
• src/lib/application/commands/__tests__/migration.test.ts
• src/lib/application/commands/pots.ts
• src/lib/application/commands/chat-sessions.ts
• src/lib/application/commands/accounts.ts
• src/lib/application/commands/sync.ts
• src/lib/application/commands/transactions.ts

pages/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

React components must not call storage or sheet services directly — they must go through command handlers in src/lib/application/commands/

State lives in Zustand stores under src/stores/ — pages must read from stores instead of DataContext or direct service calls

Use Tailwind CSS for styling with utility-first approach in page components

Files:

• pages/DashboardPage.tsx
• pages/GoalsPage.tsx
• pages/HistoryPage.tsx
• pages/AssetsPage.tsx
• pages/AccountPage.tsx
• pages/ProfilePage.tsx

src/test-setup.ts

📄 CodeRabbit inference engine (AGENTS.md)

Test setup for Vitest and React Testing Library must be in src/test-setup.ts

Files:

• src/test-setup.ts

docs/adrs/**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Architecture Decision Records (ADRs) must be stored in docs/adrs/ with sequential numbering and kebab-case filenames

ADRs must use template with sections: Context → Decision → Consequences → Status

Files:

• docs/adrs/001-layered-architecture-refactor.md
• docs/adrs/002-drop-vault-data-minimization.md

vite.config.ts

📄 CodeRabbit inference engine (AGENTS.md)

Vite and Vitest configuration must be defined in vite.config.ts

Files:

• vite.config.ts

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T21:00:30.450Z
Learning: Create feature branches with prefix `feature/*` for new features

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T21:00:30.450Z
Learning: Create refactor branches with prefix `refactor/*` for major code refactoring or UI revamp

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T21:00:30.450Z
Learning: Create bugfix branches with prefix `bugfix/*` for bug fixes

Learnt from: CR
Repo: notedwin-dev/finance-tracker-app

Timestamp: 2026-06-05T21:00:30.450Z
Learning: Use `staging` as the pre-production review branch and merge to `main` only after approval

🪛 LanguageTool

docs/issues/review-pr2.md

[grammar] ~31-~31: Ensure spelling is correct
Context: ... handleTransactionDelete removed from DataContextType. ## MEDIUM Issues ### M1 — Duplicate balanc...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

.opencode/prompts/commands-splitter.txt

[grammar] ~1-~1: Use a hyphen to join words.
Context: You are a code splitting specialist. Your task is to sp...

(QB_NEW_EN_HYPHEN)

---

[style] ~19-~19: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...everything 6. Zero behavioral changes — exact same function bodies 7. Every existing impor...

(EN_WORDINESS_PREMIUM_EXACT_SAME)

docs/issues/001-rename-and-split-computeBalanceDeltas.md

[style] ~58-~58: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... ## Decision Document - Functions use exact same logic from the original — no behavioral...

(EN_WORDINESS_PREMIUM_EXACT_SAME)

🪛 markdownlint-cli2 (0.22.1)

AGENTS.md

[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 4-4: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 4-4: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 63-63: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 123-123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 133-133: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 146-146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

docs/PLAN.md

[warning] 223-223: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 233-233: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 242-242: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

docs/adrs/001-layered-architecture-refactor.md

[warning] 32-32: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 120-120: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 141-141: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 145-145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 149-149: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 153-153: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 156-156: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 163-163: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 172-172: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 179-179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 188-188: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 197-197: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 203-203: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 211-211: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 221-221: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 229-229: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 234-234: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 243-243: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 246-246: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 249-249: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 252-252: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 255-255: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

PLAN.md

[warning] 9-9: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 38-38: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 66-66: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 120-120: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 133-133: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 166-166: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 189-189: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 259-259: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 259-259: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 271-271: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 271-271: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 283-283: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 283-283: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (72)

.gitignore (1)

7-8: LGTM!

components/history/SearchOverlay.tsx (1)

162-167: LGTM!

docs/adrs/002-drop-vault-data-minimization.md (1)

1-55: LGTM!

docs/agents/domain.md (1)

1-38: LGTM!

docs/agents/issue-tracker.md (1)

1-23: LGTM!

src/lib/application/commands.ts (1)

1-12: LGTM!

pages/HistoryPage.tsx (1)

27-27: Verify deleteTransaction handling for synthetic ${id}_in transfer IDs.

HistoryPage.tsx fabricates display IDs via ${id}_in (line 27) but forwards the passed id directly into deleteTransaction (lines 70-72). Confirm that src/lib/application/commands/transactions.ts normalizes/resolves _in IDs to the canonical transaction (or deletes the correct transfer leg); otherwise the IN-row delete may fail/no-op.

components/History.tsx (1)

174-206: ⚡ Quick win

Catch failures in batch delete/edit handlers to prevent unhandled promise rejections
handleBatchDelete / handleBatchEditSubmit only use try/finally; if batchDeleteTransaction or batchEditTransactions rejects, the async callback can leak a rejected promise from the click handler. Add an explicit catch to surface/log the error and keep batch.endSubmit() behavior.

Suggested patch

 const handleBatchDelete = useCallback(async () => {
   batch.startSubmit();
   try {
@@
       await batchDeleteTransaction(batch.selectedIds, accounts, pots, pockets, usdRate, transactions);
       batch.clearSelection();
     }
+  } catch (error) {
+    void error;
   } finally {
     batch.endSubmit();
   }
 }, [batch, batchDeleteTransaction, accounts, pots, pockets, usdRate, transactions]);

 const handleBatchEditSubmit = useCallback(async () => {
   batch.startSubmit();
   try {
@@
       false,
     );
+  } catch (error) {
+    void error;
   } finally {
     batch.endSubmit();
   }
 }, [batch, batchEditTransactions, transactions, accounts, pots, pockets, usdRate]);

PLAN.md (1)

9-18: Add language identifier to fenced code block.

The ASCII diagram fence is missing a language tag, causing markdown lint warnings. Add text as the language identifier.

📝 Suggested fix

-```
+```text
 Sheets ──→ DataProvider ──→ Stores ──→ Components

components/history/useFilteredTransactions.ts (1)

9-12: LGTM!

App.tsx (1)

4-4: LGTM!

Also applies to: 40-50

CONTEXT.md (1)

1-64: LGTM!

components/AIInsights.tsx (1)

18-18: LGTM!

Also applies to: 376-376

components/Goals.tsx (1)

4-4: LGTM!

Also applies to: 42-42

pages/AssetsPage.tsx (1)

4-5: LGTM!

Also applies to: 11-14

pages/GoalsPage.tsx (1)

3-9: LGTM!

Also applies to: 12-17, 26-31

components/AccountCard.tsx (1)

11-11: LGTM!

Also applies to: 35-35

components/Charts.tsx (1)

17-17: LGTM!

Also applies to: 876-876

components/SubscriptionManager.tsx (1)

15-15: LGTM!

Also applies to: 37-37

helpers/useMask.tsx (1)

1-52: LGTM!

opencode.json (1)

1-89: LGTM!

package.json (1)

9-11: LGTM!

Also applies to: 34-46

pages/AccountPage.tsx (1)

6-10: LGTM!

Also applies to: 34-35, 55-67, 91-91, 546-555, 619-625

pages/DashboardPage.tsx (1)

12-14: LGTM!

Also applies to: 36-40

components/BulkImportModal.tsx (1)

18-18: LGTM!

Also applies to: 106-106

docs/agents/triage-labels.md (1)

1-15: LGTM!

src/stores/__tests__/finance.store.test.ts (1)

1-107: LGTM!

src/stores/ui.store.ts (1)

1-23: LGTM!

components/GoogleDrivePicker.tsx (1)

6-10: LGTM!

Also applies to: 41-42, 48-49, 54-55, 62-63, 120-121, 130-131, 145-148

components/Profile.tsx (1)

19-24: LGTM!

Also applies to: 60-72, 289-306, 394-395, 436-472, 541-542

docs/issues/004-commands-extraction.md (1)

1-34: LGTM!

docs/issues/005-cleanup.md (1)

1-31: LGTM!

docs/issues/README.md (1)

1-24: LGTM!

src/lib/application/commands/__tests__/migration.test.ts (1)

1-247: LGTM!

src/lib/application/commands/migration.ts (1)

17-19: Confirm StorageService.saveProfile is synchronous; otherwise await it (and the similar block at 21–24)
saveProfile(cleanedProfile) is called without await while saveAccounts(...) is awaited—if saveProfile returns a Promise/is async, add await StorageService.saveProfile(cleanedProfile); to preserve persistence ordering.

docs/issues/review-pr2.md (1)

1-81: LGTM!

src/lib/application/commands/accounts.ts (1)

6-103: LGTM!

src/lib/application/commands/chat-sessions.ts (1)

6-33: LGTM!

src/lib/application/commands/subscriptions.ts (1)

6-37: LGTM!

services/coin.services.ts (1)

1-1: LGTM!

Also applies to: 44-44

services/crypto.services.ts (1)

1-135: LGTM!

services/exchange.services.ts (1)

2-2: LGTM!

Also applies to: 47-47, 151-151

services/gemini.services.ts (1)

12-12: LGTM!

Also applies to: 289-290, 408-417, 455-456, 495-499, 565-566

skills-lock.json (1)

1-137: LGTM!

services/auth.services.tsx (1)

286-303: ⚡ Quick win

Avoid using updatedP assigned inside setProfile for the post-update Sheets sync

updatedP is mutated inside the setProfile((prev) => { ... }) updater, then used immediately afterward in the sync branch. React doesn’t guarantee that updater assignment will have completed (and the updater may run more than once), so the sync logic can see stale/undefined data. Compute the next profile in a way that’s available synchronously for both persistence and cloud sync, or move the Sheets sync into a useEffect that runs when the updated profile/email state changes.

services/storage.services.ts (1)

14-14: LGTM!

Also applies to: 82-84, 90-90, 163-163, 167-167, 493-507, 510-510

src/lib/application/commands/pots.ts (1)

1-41: LGTM!

src/lib/domain/__tests__/SearchOverlay.test.tsx (1)

1-123: LGTM!

src/stores/finance.store.ts (1)

1-202: LGTM!

types.ts (1)

35-35: LGTM!

Also applies to: 86-87

src/lib/application/__tests__/commands.test.ts (1)

1-141: LGTM!

src/lib/domain/__tests__/matchesSearch.test.ts (1)

1-106: LGTM!

src/lib/domain/__tests__/migration.test.ts (1)

1-319: LGTM!

src/lib/domain/search.ts (1)

1-32: LGTM!

src/stores/__tests__/mask.store.test.ts (1)

1-21: LGTM!

src/stores/__tests__/sync.store.test.ts (1)

1-92: LGTM!

src/stores/__tests__/ui.store.test.ts (1)

1-20: LGTM!

src/lib/application/commands/sync.ts (1)

117-117: Unused loadData parameters are still present (profile, _forceUnlock).

This appears to be the same unresolved issue previously reported.

src/lib/domain/migration.ts (2)

1-2: Top-of-file temporary comment is still present in this TypeScript module.

Already reported previously.

---

4-92: LGTM!

src/lib/application/__tests__/logger.test.ts (1)

1-108: LGTM!

src/lib/application/logger.ts (1)

1-109: LGTM!

src/stores/index.ts (1)

1-12: LGTM!

src/stores/mask.store.ts (1)

1-22: LGTM!

src/lib/domain/__tests__/currency.test.ts (1)

1-33: LGTM!

src/lib/domain/currency.ts (1)

1-23: LGTM!

src/lib/domain/dates.ts (1)

1-26: LGTM!

src/stores/sync.store.ts (1)

1-67: LGTM!

src/test-setup.ts (1)

1-1: LGTM!

tsconfig.json (1)

14-16: LGTM!

vite.config.ts (1)

1-1: LGTM!

Also applies to: 8-8, 15-19, 64-69

src/lib/domain/balance.engine.ts (1)

17-20: ⚡ Quick win

Fix reset-gate date comparisons in src/lib/domain/balance.engine.ts (lines 17-20, 49-52, 78-80).
These comparisons are done on normalized date strings (lexicographic order), which can misclassify ordering due to locale/format changes or invalid normalization—skewing pot/pocket delta gating. Compare using numeric timestamps (e.g., Date(...).getTime()) and handle invalid dates explicitly (null/skip gate).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix markdownlint violations for heading spacing and fenced code metadata.

This block currently violates MD022/MD031/MD040. Add blank lines around the affected headings/fences and add a language to the fenced structure block (e.g., text).

Also applies to: 40-40, 58-58, 63-63, 123-123, 133-133, 146-146

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 4-4: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 4-4: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@AGENTS.md` around lines 3 - 4, Add required blank lines around top-level
headings and fenced code blocks and include a language identifier for fenced
blocks (for example change ``` to ```text) to satisfy MD022/MD031/MD040;
specifically, ensure there is an empty line above and below "## Project
Structure" and every fenced block, update the fences to include a language token
(e.g., text), and apply the same fixes to the other affected heading/fence
occurrences noted (around lines referenced in the review) so all headings and
code fences have proper surrounding blank lines and a fenced language.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Remove inline JSX route-section comments in this TSX file.

Please drop these comments and keep the route grouping self-descriptive via structure/naming.

Suggested cleanup

-          {/* Public Routes */}
           <Route path="/" element={<LandingPage />} />
           <Route path="/login" element={<AuthPage />} />
           <Route path="/privacy" element={<PrivacyPolicy />} />
           <Route path="/terms" element={<TermsOfService />} />

-          {/* Protected App Routes */}
           <Route
             path="/app"
             element={
@@
-          {/* fallback */}
           <Route path="/404" element={<NotFound />} />
           <Route path="*" element={<Navigate to="/404" replace />} />

As per coding guidelines, **/*.{ts,tsx}: Avoid comments — prefer self-documenting code.

Also applies to: 75-75

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@App.tsx` around lines 51 - 57, Remove the inline JSX comments like {/* Public
Routes */} and {/* Protected App Routes */} in App.tsx; instead make the
grouping self-descriptive by either extracting the sets of Route elements into
named components (e.g., PublicRoutes and ProtectedRoutes) or using clearly named
JSX wrappers/fragments so the Route entries (LandingPage, AuthPage,
PrivacyPolicy, TermsOfService, etc.) are organized without comments; ensure any
other similar inline comment nodes (including the one near Routes around the
protected section) are deleted and replaced with the descriptive structure.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Align onSave typing with awaited async behavior.

Line 12 types onSave as void, but Line 170 awaits it. This weakens the contract and can mask non-awaited persistence paths.

Proposed fix

 interface Props {
   initialAccount?: Account;
   accounts: Account[];
-  onSave: (account: Omit<Account, "userId">) => void;
+  onSave: (account: Omit<Account, "userId">) => Promise<void>;
   onDelete?: (id: string, name: string) => void;
   onClose: () => void;
 }

Also applies to: 164-171

🤖 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 `@components/AccountForm.tsx` at line 12, The onSave prop on the AccountForm
component is typed as returning void but is awaited where it's used; update the
prop signature to return a Promise (e.g., onSave: (account: Omit<Account,
"userId">) => Promise<void>) and adjust any related callers to return/propagate
a Promise as needed so awaiting the call is type-safe; look for the AccountForm
props/interface and the function using await onSave to ensure both sides match.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard the "Recalculate Balances" action when the handler is unavailable.

Line 43 makes onRecalculateBalances optional, but Lines 429-435 always render a clickable setting. When the prop is missing, this becomes a silent no-op.

Suggested fix

-						<SettingItem
-							icon={CalculatorIcon}
-							label="Recalculate Balances"
-							description="Fix account balance desyncs from history"
-							onClick={handleRecalculateBalances}
-							color="text-emerald-400"
-						/>
+						{onRecalculateBalances && (
+							<SettingItem
+								icon={CalculatorIcon}
+								label="Recalculate Balances"
+								description="Fix account balance desyncs from history"
+								onClick={handleRecalculateBalances}
+								color="text-emerald-400"
+							/>
+						)}

Also applies to: 429-435

🤖 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 `@components/Profile.tsx` around lines 43 - 44, The "Recalculate Balances"
action is rendered as an always-clickable setting even though the prop
onRecalculateBalances is optional; update the Profile component to guard
rendering and interaction by checking onRecalculateBalances before enabling the
control: either hide the control when onRecalculateBalances is undefined or
render it in a disabled state (and prevent clicks) and optionally show a
tooltip/message; ensure the click handler bound in the Recalculate Balances
setting uses the presence check (e.g., if (!onRecalculateBalances) return) and
the UI reflects disabled state so there is no silent no-op when
onRecalculateBalances is not provided.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Add language identifier to architecture diagram.

The fenced block containing the architecture diagram is missing a language tag. Add text to satisfy markdown linting and improve rendering.

📝 Suggested fix

-```
+```text
 ┌─────────────────────────────────────────────┐

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 32-32: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@docs/adrs/001-layered-architecture-refactor.md` around lines 32 - 56, The
fenced code block containing the ASCII architecture diagram is missing a
language identifier, so update the Markdown fenced block around the diagram (the
triple-backtick block in the ADR content) to include the language tag "text"
(i.e., replace ``` with ```text) so linters and renderers treat it as plain
text; locate the diagram block in the ADR (the big box starting with
"┌─────────────────────────────────────────────┐") and add the tag to the
opening fence accordingly.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Move subscription recurrence/conversion rules into domain-layer pure functions.

processSubscriptions currently embeds business rules inside application commands (date advancement, recurrence interpretation, currency conversion, dedupe behavior). Keep this module for orchestration and move rule computation to src/lib/domain/... with explicit inputs/outputs.

As per coding guidelines, src/lib/application/**/*.ts: Application layer in src/lib/application/ must orchestrate commands without business logic.

🤖 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/application/commands/sync.ts` around lines 144 - 278,
processSubscriptions mixes orchestration with business rules (date
advancement/recurrence, transaction id creation, dedupe semantics, and currency
conversion); extract pure domain functions and replace inline logic with calls:
create a pure computeNextOccurrences(sub, today, maxIterations) that returns {
nextDateStr, generatedTxDates[], bailed } (used where parseDateSafe, nextDateStr
and the while loop are), a pure buildSubscriptionTransaction(sub, date, userId)
that builds the Transaction object (used where newTx is created and
id/shopName/date are set), a pure
convertTransactionAmountForAccount(transaction, account, usdRate, rateValid)
that returns numeric amount (used where accUpdates computes amount and handles
USD/MYR conversion and rateValid checks), and a pure
dedupeTransactions(existingTxs, candidateTxs) that returns the deduped list
(used where newTxs are filtered). Keep processSubscriptions only orchestrating:
call these domain functions, update accUpdates/updatedSubs/updatedAccounts, and
persist/showToast as before.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

updatedSubs can be silently lost when all generated txs are duplicates.

If dedupedNewTxs.length === 0, the function returns before applying/persisting advanced nextPaymentDate values, so the same cycle can repeat indefinitely.

Proposed fix

+  const store = useFinanceStore.getState();
   if (dedupedNewTxs.length === 0) {
+    store.setSubscriptions(updatedSubs);
+    if (persist) {
+      StorageService.saveSubscriptions(updatedSubs);
+    }
     useSyncStore.getState().showToast(
       "No new subscription payments to apply.",
       "info",
     );
     return;
   }
@@
-  const store = useFinanceStore.getState();
   store.setTransactions(allTxs);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (dedupedNewTxs.length === 0) {
	useSyncStore.getState().showToast(
	"No new subscription payments to apply.",
	"info",
	);
	return;
	}
	const store = useFinanceStore.getState();
	if (dedupedNewTxs.length === 0) {
	store.setSubscriptions(updatedSubs);
	if (persist) {
	StorageService.saveSubscriptions(updatedSubs);
	}
	useSyncStore.getState().showToast(
	"No new subscription payments to apply.",
	"info",
	);
	return;
	}
	store.setTransactions(allTxs);

🤖 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/application/commands/sync.ts` around lines 226 - 232, When
dedupedNewTxs is empty the code returns early and never persists the advanced
nextPaymentDate values held in updatedSubs; ensure updatedSubs is applied even
when dedupedNewTxs.length === 0 by invoking the same persistence/update routine
used for non-empty cases (i.e., call the function that saves/applies
subscription updates for updatedSubs) before returning, then show the
no-new-transactions toast via useSyncStore.getState().showToast as currently
done.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Move domain rules out of the application command.

The command currently embeds business logic (delta computation orchestration details, subscription next-date progression, cross-currency rule checks) instead of only orchestrating dependencies.

As per coding guidelines: src/lib/application/**/*.ts: Application layer in src/lib/application/ must orchestrate commands without business logic.

Also applies to: 174-216

🤖 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/application/commands/transactions.ts` around lines 44 - 72, This
command embeds domain rules (cross-currency check and delta-computation
orchestration) inside application logic: extract the business logic into domain
functions/services and call them from the command; specifically move the
isCrossCurrency logic and the usdRate/cross-currency validation into a
domain/validation function (e.g., TransactionDomain.validateExchangeRate(tx,
txAccount, toAccount, usdRate)) and move the applyDeltas orchestration into a
domain service (e.g., TransactionDomain.computeDeltas(tx, factor, accounts,
pots, pockets) that internally uses computeAccountTransactionAmount,
computeBudgetConsumption, computeSavingsMovement) and then replace the inline
isCrossCurrency/if-check and applyDeltas usage in this file with calls to those
domain functions (keeping showToast usage only for presenting errors returned by
the domain validation).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Await local persistence and move store mutation behind a single success boundary.

Line 150 (and similar paths at Line 301, Line 457, Line 584) fire async saves without await after mutating store state. This can show success while persistence fails, leaving local/cloud/state divergence.

Suggested direction

-  store.setTransactions(updatedTransactions);
-  if (accountUpdates.size > 0) store.setAccounts(updatedAccounts);
-  if (potUpdates.size > 0) store.setPots(updatedPots);
-  if (pocketUpdates.size > 0) store.setPockets(updatedPockets);
-
-  StorageService.saveTransactions(updatedTransactions);
-  if (accountUpdates.size > 0) StorageService.saveAccounts(updatedAccounts);
-  if (potUpdates.size > 0) StorageService.savePots(updatedPots);
-  if (pocketUpdates.size > 0) StorageService.savePockets(updatedPockets);
+  await Promise.all([
+    StorageService.saveTransactions(updatedTransactions),
+    ...(accountUpdates.size > 0 ? [StorageService.saveAccounts(updatedAccounts)] : []),
+    ...(potUpdates.size > 0 ? [StorageService.savePots(updatedPots)] : []),
+    ...(pocketUpdates.size > 0 ? [StorageService.savePockets(updatedPockets)] : []),
+  ]);
+
+  store.setTransactions(updatedTransactions);
+  if (accountUpdates.size > 0) store.setAccounts(updatedAccounts);
+  if (potUpdates.size > 0) store.setPots(updatedPots);
+  if (pocketUpdates.size > 0) store.setPockets(updatedPockets);

Also applies to: 299-302, 456-458, 583-585

🤖 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/application/commands/transactions.ts` around lines 145 - 153, The
code currently mutates the in-memory store via store.setTransactions /
store.setAccounts / store.setPots / store.setPockets and then fires
StorageService.saveTransactions / saveAccounts / savePots / savePockets without
awaiting, which can expose UI/state before persistence succeeds; change the flow
so you first call and await the corresponding StorageService.save* calls, check
for success (handle or surface errors), and only after successful persistence
perform the store.setTransactions / store.setAccounts / store.setPots /
store.setPockets mutations (apply the same fix for the other identical blocks
that call StorageService.save* and store.set* in this file).

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add historical-transaction test cases for budget and savings calculators.

The suite asserts isHistorical behavior for computeAccountTransactionAmount (Line 62) but not for computeBudgetConsumption and computeSavingsMovement. Add those assertions to lock in parity and prevent regressions.

Proposed test additions

 describe("computeBudgetConsumption", () => {
+  it("returns empty map for historical transactions", () => {
+    const result = computeBudgetConsumption(
+      makeTx({ type: TransactionType.EXPENSE, potId: "pot1", isHistorical: true }),
+      1,
+      [makePot()],
+    );
+    expect(result.size).toBe(0);
+  });
@@
 describe("computeSavingsMovement", () => {
+  it("returns empty map for historical transactions", () => {
+    const result = computeSavingsMovement(
+      makeTx({ type: TransactionType.INCOME, savingPocketId: "pocket1", isHistorical: true }),
+      1,
+      [makePocket()],
+    );
+    expect(result.size).toBe(0);
+  });

Also applies to: 266-311

🤖 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__/balance.engine.test.ts` around lines 219 - 264, Add
tests mirroring the existing isHistorical assertion for
computeAccountTransactionAmount: create test cases for computeBudgetConsumption
and computeSavingsMovement that pass transactions with isHistorical: true (using
makeTx) and assert the result flips sign (i.e., consumption/movement is
multiplied by -1) compared to the non-historical case; also include a historical
case for when no potId and when transaction is before pot reset date to ensure
size is 0/undefined as appropriate. Reference computeBudgetConsumption,
computeSavingsMovement, makeTx, makePot and TransactionType to locate where to
add these assertions.