fix(cleanup): address post-merge code review findings#12
Merged
Conversation
Code:
- History.handleSwipeDelete: only clear swipe state on successful delete
- useAppInit: catch exchange/crypto fetch failures (no more unhandled rejections)
- MainLayout: add aria-pressed to desktop Mask Mode toggle
- ProfilePage: recursive stripSensitive (deny-list regex) for export
- commands/balance: Promise.all + await for save trio
- commands/chat-sessions: await saveChatSessions + save-before-set
- commands/pockets: clear toSavingPocketId on delete
- commands/pots, subscriptions: save-before-set pattern
- commands/sync: use activeProfile.syncChatToSheets (post-merge value)
- domain/migration: updatedAt parameter (pure function)
- domain/search: extract matchesSearch from useFilteredTransactions
Tests:
- matchesSearch.test: import from domain; transfer fixture uses TRANSFER type
- migration.test: updatedAt test asserts on provided timestamp
Docs:
- AGENTS.md: drop context/ ref, update layer-isolation rule, matchesSearch row
- ADR-001: split ui.store.ts row (State | Actions columns)
- issues/003-004-005: update to 147 tests, mark subtasks complete
- review-pr2: typo fix
Config:
- opencode.json: portable plugin path (./opencode-power-pack)
- .opencode/prompts/{page,state}-migrator: reflect current architecture
Skipped: ADR Status reorder (consistent across repo), balance.engine
dateNormalizer (larger refactor), accounts.ts save order (matches test
convention), transactions.ts comment extraction (699-line refactor),
migration.ts header comment (carries removal criterion).
147 tests pass, npx tsc --noEmit clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…Page conflict Conflict in pages/ProfilePage.tsx: HEAD used a deny-list regex (11 patterns) for export sanitization; staging's PR #11 used an allow-list for profile (11 fields) + deny-list of { details, isEncrypted } for accounts. Resolved with the deny-list approach but extended the regex to match the logger's SENSITIVE_KEYS set plus common identifier patterns (apikey lowercase, vite_* env keys, totpSecret, encryptionKey, biometricCred[Id|Ids], devices, cardNumber, cvv, expiry, holderName, accountNumber, pin, ssn, taxId, iban, routingNumber, swift). Why deny-list over allow-list: - Export is user-initiated data backup; preserve as much as possible - UserProfile type has 16+ fields; the 11-field allow-list silently dropped photoUrl, updatedAt, isSecurityEnabled, isVaultEnabled, totpEnabled, biometricEnabled — that's data loss for a backup feature - Recursive redact() handles nested objects uniformly (transactions referencing accounts, goals, pockets) without per-entity allow-lists - The OWASP 'default deny' rule applies to auth/authz on untrusted input, not to user data export 153 tests pass, npx tsc --noEmit clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/pockets.ts (1)
23-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReverse the persistence order to match the save-before-set pattern.
saveSavingPocketupdates the store before awaiting storage, which is inconsistent with the other command handlers in this PR and creates a window where the UI reflects unsaved state. If the save fails, the store will contain data that was never persisted.🔄 Proposed fix
- store.setPockets(updated); await StorageService.savePockets(updated); + store.setPockets(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/pockets.ts` around lines 23 - 24, The code calls store.setPockets(updated) before awaiting persistence, which can leave the UI showing unsaved state if saving fails; change the order so you first await StorageService.savePockets(updated) and only after a successful save call store.setPockets(updated) (modify the saveSavingPocket handler that currently uses store.setPockets and StorageService.savePockets to reverse those calls and preserve existing error handling).
🤖 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/History.tsx`:
- Around line 167-170: Remove the inline comment inside the empty catch block
that follows swipe.setSwipedId(null); in the component so the catch remains
empty and self-explanatory; locate the try/catch that calls
swipe.setSwipedId(null) and delete the comment line between catch { and } to
comply with the guideline preferring self-documenting code.
In `@helpers/useAppInit.tsx`:
- Around line 40-47: The warning message is misleading because getUSDToMYRRate()
failures do not set a fallback of 1; update the catch block that calls
logger.warn to accurately state that the USD/MYR rate fetch failed and the store
will retain its current value (via useFinanceStore.getState().setUsdRate /
setExchangeRate not being called), or alternatively set an explicit fallback by
calling useFinanceStore.getState().setUsdRate(1) and setExchangeRate(...) if you
prefer a true default; modify the message accordingly so it references the
actual behavior.
- Around line 48-54: The warning message in the getCryptoPrices promise catch is
misleading — it says "using empty list" but the code leaves useFinanceStore's
initial cryptoPrices ({ BTC: 0, ETH: 0 }) in place; update the logger.warn call
in the getCryptoPrices().catch block to accurately describe the fallback (e.g.,
"Failed to fetch crypto prices; retaining initial prices" or include the actual
initial/default state) and keep the existing call to
useFinanceStore.getState().setCryptoPrices untouched unless you intend to
explicitly set an empty list.
In `@opencode.json`:
- Line 4: The opencode plugin entry
"opencode-power-pack@git+file:./opencode-power-pack" in opencode.json points to
a missing local directory; update the plugin reference or CI/setup to ensure the
plugin exists: either change the entry in opencode.json to the correct existing
location/name of the package, replace the git+file URI with a valid git URL or
registry version, or add steps to your setup/CI to fetch/create
./opencode-power-pack (ensure it is not gitignored) so the plugin referenced by
opencode.json can be resolved at runtime.
In `@pages/ProfilePage.tsx`:
- Around line 42-54: stripSensitive currently only filters top-level keys
(SENSITIVE_KEYS) but does not recurse into nested objects/arrays, so sensitive
fields like meta.apiKey remain; modify stripSensitive to recursively process
object property values and array elements: when handling an object (used in the
Object.entries/Object.fromEntries block for stripSensitive), after filtering out
keys matching SENSITIVE_KEYS, map each retained value through stripSensitive so
nested objects/arrays are cleaned too (similarly ensure Array.isArray branch
maps elements via stripSensitive); follow the same value-recursion approach used
by logger.redact to avoid leaving nested sensitive fields.
In `@src/lib/application/commands/pockets.ts`:
- Around line 40-48: The map callback that builds updatedTransactions
incorrectly early-returns after clearing one field so a transaction with both
savingPocketId and toSavingPocketId equal to id only has the first cleared;
update the map logic in the existingTransactions.map callback to always produce
a new transaction object and conditionally set both savingPocketId and
toSavingPocketId to null when they equal id (e.g., create let newT = { ...t };
if (newT.savingPocketId === id) newT.savingPocketId = null; if
(newT.toSavingPocketId === id) newT.toSavingPocketId = null; return newT) so
both references are removed.
In `@src/lib/domain/__tests__/migration.test.ts`:
- Around line 183-187: Add a new unit test in migration.test.ts that calls
migrateSchemaV1toV2([], baseProfile) without the third parameter and verifies
profile.updatedAt is set to a valid ISO timestamp: capture a `before` and
`after` ISO timestamp around the call, assert profile.updatedAt is defined and a
string, match it against an ISO timestamp regex, and assert it falls between
`before` and `after`; reference the migrateSchemaV1toV2 function and the
profile.updatedAt property in the assertions.
In `@src/lib/domain/migration.ts`:
- Line 58: The domain function's default parameter new Date().toISOString()
introduces a side effect; remove the default and make the timestamp an explicit
required parameter (change the updatedAt parameter in migrateSchemaV1toV2 in
src/lib/domain/migration.ts to a plain string without a default). Then update
all callers (e.g., where migrateSchemaV1toV2 is invoked in
src/lib/application/commands/migration.ts) to pass new Date().toISOString() at
call sites, ensuring the domain layer remains pure and all time dependencies are
injected by the caller.
---
Outside diff comments:
In `@src/lib/application/commands/pockets.ts`:
- Around line 23-24: The code calls store.setPockets(updated) before awaiting
persistence, which can leave the UI showing unsaved state if saving fails;
change the order so you first await StorageService.savePockets(updated) and only
after a successful save call store.setPockets(updated) (modify the
saveSavingPocket handler that currently uses store.setPockets and
StorageService.savePockets to reverse those calls and preserve existing error
handling).
🪄 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: 6e856a8d-c7cf-448c-b7aa-99a94305f7a7
📒 Files selected for processing (24)
.opencode/prompts/page-migrator.txt.opencode/prompts/state-migrator.txtAGENTS.mdcomponents/History.tsxcomponents/history/useFilteredTransactions.tsdocs/adrs/001-layered-architecture-refactor.mddocs/issues/003-privacy-store.mddocs/issues/004-commands-extraction.mddocs/issues/005-cleanup.mddocs/issues/review-pr2.mdhelpers/useAppInit.tsxlayouts/MainLayout.tsxopencode.jsonpages/ProfilePage.tsxsrc/lib/application/commands/balance.tssrc/lib/application/commands/chat-sessions.tssrc/lib/application/commands/pockets.tssrc/lib/application/commands/pots.tssrc/lib/application/commands/subscriptions.tssrc/lib/application/commands/sync.tssrc/lib/domain/__tests__/matchesSearch.test.tssrc/lib/domain/__tests__/migration.test.tssrc/lib/domain/migration.tssrc/lib/domain/search.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comments — prefer self-documenting code with clear naming and structure.
Files:
src/lib/application/commands/chat-sessions.tssrc/lib/domain/migration.tssrc/lib/application/commands/sync.tssrc/lib/domain/__tests__/migration.test.tssrc/lib/application/commands/balance.tssrc/lib/domain/search.tslayouts/MainLayout.tsxsrc/lib/application/commands/subscriptions.tspages/ProfilePage.tsxsrc/lib/domain/__tests__/matchesSearch.test.tssrc/lib/application/commands/pockets.tshelpers/useAppInit.tsxcomponents/History.tsxsrc/lib/application/commands/pots.tscomponents/history/useFilteredTransactions.ts
src/lib/domain/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Domain layer functions must be pure functions only — no React imports, no side effects, no imports from
services/orstores/. All dependencies must be explicit parameters.Use
TransactionTypeenum from types for type discrimination in domain functions.Keep domain modules organized as one module per file with co-located
__tests__/directories insrc/lib/domain/.
Files:
src/lib/domain/migration.tssrc/lib/domain/__tests__/migration.test.tssrc/lib/domain/search.tssrc/lib/domain/__tests__/matchesSearch.test.ts
src/**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate unit tests in
__tests__/directories adjacent to source files, using Vitest.
Files:
src/lib/domain/__tests__/migration.test.tssrc/lib/domain/__tests__/matchesSearch.test.ts
{components,pages,layouts}/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind CSS utility-first styling for all React components.
Files:
layouts/MainLayout.tsxpages/ProfilePage.tsxcomponents/History.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Domain functions use Vitest for pure unit tests without jsdom. Zustand stores use Vitest + jsdom for store action tests. React components use Vitest + React Testing Library + jsdom for component tests.
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Create new Git branches from `main` with prefixes: `feature/xxx` for new features, `bugfix/xxx` for bug fixes, `refactor/xxx` for major refactoring or UI revamp.
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Push feature branches and open PRs to the `staging` branch for review before merging to `main` for production deployment.
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Document significant architectural decisions in `docs/adrs/` using the ADR template (Context → Decision → Consequences → Status), numbered sequentially.
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Read the latest ADR in `docs/adrs/` before making architectural changes to understand current decisions and constraints.
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Organize page-level components in `pages/` directory and feature-specific React components in feature folders within `components/` (e.g., `components/history/`).
Learnt from: CR
Repo: notedwin-dev/finance-tracker-app
Timestamp: 2026-06-05T17:53:15.998Z
Learning: Use `VITE_BACKEND_API_URL`, `VITE_GEMINI_API_KEY`, and `VITE_GOOGLE_API_KEY` environment variables for external service configuration in `.env` files.
🔇 Additional comments (23)
src/lib/domain/search.ts (1)
3-31: LGTM!components/history/useFilteredTransactions.ts (1)
10-12: LGTM!src/lib/domain/__tests__/matchesSearch.test.ts (2)
2-2: LGTM!
92-99: LGTM!layouts/MainLayout.tsx (1)
246-248: LGTM!.opencode/prompts/page-migrator.txt (1)
1-12: LGTM!.opencode/prompts/state-migrator.txt (1)
1-12: LGTM!AGENTS.md (3)
5-28: LGTM!
77-82: LGTM!
89-97: LGTM!docs/adrs/001-layered-architecture-refactor.md (1)
108-114: LGTM!docs/issues/003-privacy-store.md (1)
1-26: LGTM!docs/issues/004-commands-extraction.md (1)
23-30: LGTM!docs/issues/005-cleanup.md (1)
15-27: LGTM!docs/issues/review-pr2.md (1)
1-6: LGTM!src/lib/application/commands/balance.ts (1)
73-80: LGTM!src/lib/application/commands/chat-sessions.ts (2)
17-18: LGTM!
30-31: LGTM!src/lib/application/commands/pots.ts (2)
25-26: LGTM!
38-39: LGTM!src/lib/application/commands/subscriptions.ts (2)
21-22: LGTM!
34-35: LGTM!src/lib/application/commands/sync.ts (1)
547-547: LGTM!
Code:
- History.handleSwipeDelete: drop inline comment from empty catch
block (project rule: no comments)
- useAppInit: accurate warning messages — store retains initial
usdRate (0) / exchangeRate (null) / cryptoPrices ({ BTC: 0, ETH: 0 })
on fetch failure, not '1' or 'empty list' as previously stated
- ProfilePage.stripSensitive: recurse into retained values so nested
sensitive fields (meta.apiKey, accounts[].geminiApiKey) are caught
- commands/pockets.saveSavingPocket: persist before setPockets to match
the deleteSavingPocket pattern (UI never shows unsaved state)
- commands/pockets.deleteSavingPocket: always build new tx object so
both savingPocketId and toSavingPocketId are cleared when a pocket
is deleted (was: early return after first match)
- domain/migration: drop default for updatedAt param (was a side effect
in a pure function); callers in commands/migration now pass
new Date().toISOString() explicitly
- domain/__tests__/migration: update existing tests to pass FIXED_TS as
required third argument
Skipped:
- opencode.json plugin path: ./opencode-power-pack directory exists in
the repo (verified) — finding invalid
153 tests pass, npx tsc --noEmit clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to PR #10 and PR #11. Addresses the high/medium findings from the post-merge code review without changing behavior.
Code
History.handleSwipeDelete: only clear swipe state on successful delete (was: clear on both success and failure)useAppInit:.catch()handlers ongetUSDToMYRRateandgetCryptoPrices(unhandled-rejection log noise)MainLayout:aria-pressed={maskMode}on desktop Mask Mode toggle (mobile already had it)ProfilePage: recursivestripSensitivedeny-list regex for export (handles nested objects; replaces 2-key destructure)commands/balance.ts:Promise.all+awaitfor the saveAccounts/savePots/savePockets triocommands/chat-sessions.ts:await+ save-before-setcommands/pockets.ts: cleartoSavingPocketIdon pocket delete (was: onlysavingPocketId)commands/pots.ts,subscriptions.ts: save-before-set (persist first, then store; matches new convention)commands/sync.ts: useactiveProfile.syncChatToSheetsat L536 (was:profile.syncChatToSheetswhich ignored the cloud-merged value)domain/migration.ts:updatedAtparameter (pure function; defaults tonew Date().toISOString())domain/search.ts: extractmatchesSearchto a pure domain module;useFilteredTransactionsre-exports for back-compatTests
matchesSearch.test.ts: import from domain; transfer fixture now usesTRANSFERtypemigration.test.ts:updatedAttest asserts on the provided timestampDocs
AGENTS.md: dropcontext/reference, update layer-isolation rule,matchesSearchrow points tosrc/lib/domain/search.tsADR-001: splitui.store.tsrow into State / Actions columnsissues/003-004-005: update to 147 tests, mark completed subtasksreview-pr2.md: typocommmit→commitConfig
opencode.json: portable plugin path./opencode-power-pack(was: absolute Windows path).opencode/prompts/{page,state}-migrator.txt: reflect current architecture (no DataProvider/vault)Skipped (with reasons)
balance.engine.tsdateNormalizerparam + historical skip in budget/savings — larger refactor with behavior change riskaccounts.tssave order — current order matches the migration test convention (entity before derived)sync.tsdropprofile/_forceUnlockfromloadData—_forceUnlockhas the standard TS underscore prefix;profileis usedtransactions.tsextract inline comments → helpers — 699-line refactor, deferredmigration.tsheader comment — comment carries the ADR-002 removal criterion, worth keepingValidation
npx tsc --noEmit— cleannpm test— 13 files, 147 tests passCloses the high/medium items from the post-merge code review of PR #10 / PR #11.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation