Conversation
…encryption and management
…nse queue for Gmail notifications
…, services, and UI components
Contributor
|
Greetings, @ali-ahnaf. Thy scroll hath arrived unblemished — the runes align and no conflict bars the path. The council shall now convene over its contents, weighing each incantation by candlelight. Tarry a while, brave adventurer; we shall investigate and return to thee with our verdict. |
📝 WalkthroughWalkthroughThe PR replaces server-side OpenAI transaction parsing with client-side OpenRouter processing, encrypted per-user credentials, and a Gmail pending-expense review queue. It adds backend storage and APIs, browser crypto helpers, dashboard review components, wizard integration, and removes obsolete prompt, usage, and extractor endpoints. ChangesAI credentials and client-side processing
Pending Gmail expense workflow
AI flow migration and cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
The quest is sealed, @ali-ahnaf! Thy incantations have been woven into the main path. The realm grows stronger for thy contribution — accept the gratitude of this humble keeper of the codex. May thy commits forever merge cleanly, brave adventurer 💰 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
packages/ui/src/lib/crypto/ai-key.ts (1)
24-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider raising
DEFAULT_KDF_ITERATIONSto match current OWASP guidance.PBKDF2-HMAC-SHA256: 600,000 iterations (recommended) per the OWASP Password Storage Cheat Sheet; 310,000 iterations reflects the older (2021) recommendation. Since
deriveKekalready accepts a per-credentialiterationsvalue for backward compatibility, bumping the default for new salts is low-risk.🤖 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 `@packages/ui/src/lib/crypto/ai-key.ts` at line 24, Update DEFAULT_KDF_ITERATIONS to 600,000 to align new key derivations with current OWASP PBKDF2-HMAC-SHA256 guidance. Preserve deriveKek’s per-credential iterations override so existing credentials remain backward compatible.packages/ui/src/components/LogResourceModal.test.tsx (1)
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the new AI parse outcome branches.
Mocks are wired up for
getAiCredentialStatus/useDekSession/decryptKey/chat, but every test usesuserId: null, soparseTransactionPrompt's branches (needs-ai-setup, dek-loading, dek-unavailable, unparseable, and success name→id mapping) are never actually exercised — only enough to satisfy imports. Given this is the most complex new logic in the cohort, consider adding cases with a non-nulluserIdthat drive each outcome.🤖 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 `@packages/ui/src/components/LogResourceModal.test.tsx` around lines 1 - 34, Add LogResourceModal tests using a non-null userId that exercise parseTransactionPrompt outcomes for needs-ai-setup, dek-loading, dek-unavailable, unparseable input, and successful parsing with name-to-ID mapping. Configure the existing getAiCredentialStatus, useDekSession, decryptKey, and chat mocks per scenario, and assert the corresponding UI/result behavior for each branch.packages/ui/src/components/LogResourceModal.tsx (1)
96-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "resolve OpenRouter key" sequence into a shared hook. Four call sites independently reimplement the same dekLoading/dek/credential-status/decrypt flow; a single shared helper would remove the duplication and reduce the chance one call site drifts (e.g. missing the
selectedModelcheck) from the others.
packages/ui/src/components/LogResourceModal.tsx#L96-L121: replace the credential-check portion ofparseTransactionPromptwith a call to a shared helper (e.g.useResolvedAiCredential()returning{ apiKey, model }or throwing a typed error).packages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsx#L74-L92: replace the same block inhandleParsewith the shared helper.packages/ui/src/components/wizard/WizardChatSheet.tsx#L104-L119: replace the same block inaskWizardwith the shared helper.packages/ui/src/app/settings/google-oauth/TestExtractModal.tsx#L73-L91: replace the same block inhandleTestwith the shared helper.🤖 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 `@packages/ui/src/components/LogResourceModal.tsx` around lines 96 - 121, Extract the repeated OpenRouter credential resolution into a shared useResolvedAiCredential helper that consistently validates dekLoading, dek, hasKey, selectedModel, keyCiphertext, and keyIv, decrypts the key, and returns the resolved apiKey and model or a typed error. Replace the duplicated credential-check blocks in packages/ui/src/components/LogResourceModal.tsx lines 96-121, packages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsx lines 74-92, packages/ui/src/components/wizard/WizardChatSheet.tsx lines 104-119, and packages/ui/src/app/settings/google-oauth/TestExtractModal.tsx lines 73-91 with this helper, preserving each caller’s existing outcome or error handling.packages/ui/src/app/settings/ai/ModelPicker.tsx (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared status/error banner component. The same boxed-banner Tailwind class combination (
font-mono text-label-caps ... border-4 border-black bg-surface-container p-3) is copy-pasted across both files for error, success, and status messages, so any future style tweak (spacing, border, dark mode, etc.) has to be updated in every call site.
packages/ui/src/app/settings/ai/ModelPicker.tsx#L57-L57: use a shared<Banner variant="error">(or similar) instead of the inline error<p>.packages/ui/src/app/settings/ai/page.tsx#L213-L220: use the same shared component for the "key saved"/"no key saved" status banner.packages/ui/src/app/settings/ai/page.tsx#L222-L222: use the shared error-variant banner forstatusError.packages/ui/src/app/settings/ai/page.tsx#L245-L245: use the shared success-variant banner for the "key saved" message.packages/ui/src/app/settings/ai/page.tsx#L247-L247: use the shared banner (or a dedicated note variant) for the password-reset warning.packages/ui/src/app/settings/ai/page.tsx#L262-L262: use the shared error-variant banner formodelError.🤖 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 `@packages/ui/src/app/settings/ai/ModelPicker.tsx` at line 57, Extract a shared Banner component for the repeated boxed status/error styling, supporting error, success, status, and note variants as needed. Replace the inline banner in ModelPicker.tsx:57 and each listed banner in packages/ui/src/app/settings/ai/page.tsx at lines 213-220, 222, 245, 247, and 262 with the shared component, preserving each message and its appropriate variant.
🤖 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 `@packages/api/src/repositories/pending-gmail-expense.repository.ts`:
- Around line 42-47: The read-before-write flow in insertIfNotExists must become
atomic: use a database insert-on-conflict operation that returns null for
duplicate userId/gmailMessageId records while preserving the withDeleted: true
replay behavior; update
packages/api/src/repositories/pending-gmail-expense.repository.ts lines 42-47.
Replace findByUserId followed by save in
packages/api/src/repositories/user-ai-credential.repository.ts lines 37-45 with
a database-level upsert keyed by userId.
In `@packages/api/src/routes/ai-credentials/put-ai-credentials.route.ts`:
- Line 11: Update the kdfIterations validation in the AI credentials request
schema to enforce the shared, versioned PBKDF2 minimum and maximum (or supported
fixed value) instead of only min(1). Reuse the existing shared constants or
configuration symbols, and ensure the bounded value is validated before it can
be stored or returned to the browser.
In `@packages/api/src/routes/transactions/post-transaction.route.ts`:
- Line 17: Propagate isCommitted through the shared transaction contract: add it
to CreateTransactionInput in the shared contracts package, then update
ProfileApi.createTransaction to accept and use that shared DTO instead of a
locally defined payload type. Ensure the API and UI do not redeclare the request
DTO.
In `@packages/ui/src/app/change-password/page.tsx`:
- Line 50: The password-change flow must re-wrap the AI credential before
reporting success. Replace the stale TODO near the password-change handler with
a recoverable sequence that derives a KEK from the new password, re-wraps the
DEK using the existing crypto helpers, and updates wrappedDek and salt through
the ai-credentials endpoint; surface any failure and do not show success when
rewrapping or persistence fails.
In `@packages/ui/src/app/settings/ai/ModelPicker.tsx`:
- Around line 17-20: Update isCuratedMatch to match only when the model ID
contains a curated ID, removing the reverse curatedId.includes check. In the
useEffect warning path, replace its separate matching logic with isCuratedMatch
so picker filtering and warnings remain consistent.
In `@packages/ui/src/app/settings/ai/page.tsx`:
- Around line 3-16: Update the React imports in the AI settings page to include
a type-only FormEvent import, then annotate the form handler event parameter
with FormEvent instead of React.FormEvent. Keep the existing handler behavior
unchanged.
In `@packages/ui/src/app/settings/page.tsx`:
- Line 183: Update the Attributions link’s description element near the row
using the existing text-on-surface-variant class to also include
group-hover:text-on-primary, matching the hover styling of the other row
descriptions while preserving its current base text color.
In `@packages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsx`:
- Around line 106-116: The resolve flow around createTransaction and
deletePendingExpense is not retry-safe: a successful transaction creation
followed by delete failure can create duplicates. Reconcile these operations
through a single atomic backend create-and-resolve endpoint if available;
otherwise track the successful creation for item.id and, on retries, skip
createTransaction and retry only deletePendingExpense before calling onResolved.
In `@packages/ui/src/components/pending-expenses/PendingExpensesPanel.tsx`:
- Around line 27-35: Update fetchPending to clear the existing error when a
fetch succeeds, ensuring a later successful re-fetch removes any stale error
banner while preserving the current item and tag updates and error handling.
In `@packages/ui/src/lib/ai/openrouter.ts`:
- Around line 99-153: Add an AbortController-based timeout to both fetch calls
in chat() and listModels(), passing each controller’s signal to fetch and
aborting after the configured timeout. Ensure the timeout is cleared when each
request completes, while preserving existing response parsing and error
handling.
In `@packages/ui/src/lib/crypto/dek-session.ts`:
- Around line 130-151: Update getSessionDek so hydration failures from
importRawDek do not permanently retain a rejected hydrationPromise: clear
hydrationPromise in the failure path, reset or preserve hydration state so a
later call can retry, and ensure the rejected attempt is propagated to its
caller. Keep successful hydration behavior, including currentDek assignment and
notify(), unchanged.
In `@README.md`:
- Line 233: Update the privacy statement in the Wizard Assistant description to
remove the claim that nothing is sent to Pocket Pixel’s servers, since
getPendingExpenseEmail and confirmed transactions use Pocket Pixel APIs. Limit
the claim to the client-side OpenRouter key and AI-processing requests, while
preserving the existing feature description.
---
Nitpick comments:
In `@packages/ui/src/app/settings/ai/ModelPicker.tsx`:
- Line 57: Extract a shared Banner component for the repeated boxed status/error
styling, supporting error, success, status, and note variants as needed. Replace
the inline banner in ModelPicker.tsx:57 and each listed banner in
packages/ui/src/app/settings/ai/page.tsx at lines 213-220, 222, 245, 247, and
262 with the shared component, preserving each message and its appropriate
variant.
In `@packages/ui/src/components/LogResourceModal.test.tsx`:
- Around line 1-34: Add LogResourceModal tests using a non-null userId that
exercise parseTransactionPrompt outcomes for needs-ai-setup, dek-loading,
dek-unavailable, unparseable input, and successful parsing with name-to-ID
mapping. Configure the existing getAiCredentialStatus, useDekSession,
decryptKey, and chat mocks per scenario, and assert the corresponding UI/result
behavior for each branch.
In `@packages/ui/src/components/LogResourceModal.tsx`:
- Around line 96-121: Extract the repeated OpenRouter credential resolution into
a shared useResolvedAiCredential helper that consistently validates dekLoading,
dek, hasKey, selectedModel, keyCiphertext, and keyIv, decrypts the key, and
returns the resolved apiKey and model or a typed error. Replace the duplicated
credential-check blocks in packages/ui/src/components/LogResourceModal.tsx lines
96-121,
packages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsx lines
74-92, packages/ui/src/components/wizard/WizardChatSheet.tsx lines 104-119, and
packages/ui/src/app/settings/google-oauth/TestExtractModal.tsx lines 73-91 with
this helper, preserving each caller’s existing outcome or error handling.
In `@packages/ui/src/lib/crypto/ai-key.ts`:
- Line 24: Update DEFAULT_KDF_ITERATIONS to 600,000 to align new key derivations
with current OWASP PBKDF2-HMAC-SHA256 guidance. Preserve deriveKek’s
per-credential iterations override so existing credentials remain backward
compatible.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f9b2bac-440a-4a84-a0db-1315ecc68ee1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (74)
README.mddocumentation/ai-watcher-plan.mddocumentation/multi-watcher-plan.mdpackages/api/.env.examplepackages/api/package.jsonpackages/api/src/data-source.tspackages/api/src/entities/PendingGmailExpense.entity.tspackages/api/src/entities/UserAiCredential.entity.tspackages/api/src/entities/VaultGmailWatcher.entity.tspackages/api/src/index.tspackages/api/src/migrations/1784875736698-AddUserAiCredentials.tspackages/api/src/migrations/1784885759673-AddPendingGmailExpenses.tspackages/api/src/repositories/index.tspackages/api/src/repositories/pending-gmail-expense.repository.tspackages/api/src/repositories/user-ai-credential.repository.tspackages/api/src/routes/ai-credentials.routes.tspackages/api/src/routes/ai-credentials/get-ai-credentials.route.tspackages/api/src/routes/ai-credentials/put-ai-credentials-model.route.tspackages/api/src/routes/ai-credentials/put-ai-credentials.route.tspackages/api/src/routes/pending-expenses.routes.tspackages/api/src/routes/pending-expenses/delete-pending-expense.route.tspackages/api/src/routes/pending-expenses/get-pending-expense-email.route.tspackages/api/src/routes/pending-expenses/get-pending-expenses.route.tspackages/api/src/routes/prompt.routes.tspackages/api/src/routes/prompt/get-usage.route.tspackages/api/src/routes/prompt/post-prompt.route.tspackages/api/src/routes/transactions/post-transaction.route.tspackages/api/src/routes/vault-watchers.routes.tspackages/api/src/routes/vault-watchers/test-extract.route.tspackages/api/src/routes/wizard.routes.tspackages/api/src/routes/wizard/post-chat.route.tspackages/api/src/services/gmail-ai-extractor.service.tspackages/api/src/services/gmail.service.tspackages/api/src/services/index.tspackages/api/src/services/pending-gmail-expense.service.tspackages/api/src/services/prompt.service.tspackages/api/src/services/user-ai-credential.service.tspackages/api/src/services/wizard.service.tspackages/api/src/tests/gmail-ai-extractor.service.test.tspackages/api/src/tests/gmail.service.test.tspackages/api/src/tests/pending-gmail-expense.service.test.tspackages/api/src/tests/prompt.service.test.tspackages/api/src/tests/user-ai-credential.service.test.tspackages/api/src/tests/wizard.service.test.tspackages/shared/src/contracts/ai-credentials.tspackages/shared/src/contracts/ai.tspackages/shared/src/contracts/index.tspackages/shared/src/contracts/pending-expenses.tspackages/shared/src/contracts/wizard.tspackages/ui/src/app/change-password/page.tsxpackages/ui/src/app/page.tsxpackages/ui/src/app/settings/ai/ModelPicker.tsxpackages/ui/src/app/settings/ai/page.tsxpackages/ui/src/app/settings/google-oauth/TestExtractModal.tsxpackages/ui/src/app/settings/page.tsxpackages/ui/src/app/signin/page.tsxpackages/ui/src/app/signup/page.tsxpackages/ui/src/app/stats/page.tsxpackages/ui/src/components/LogResourceModal.test.tsxpackages/ui/src/components/LogResourceModal.tsxpackages/ui/src/components/index.tspackages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsxpackages/ui/src/components/pending-expenses/PendingExpensesPanel.tsxpackages/ui/src/components/wizard/WizardChatSheet.tsxpackages/ui/src/hooks/useDekSession.tspackages/ui/src/lib/ai/gmail-extractor.tspackages/ui/src/lib/ai/openrouter.tspackages/ui/src/lib/ai/wizard.tspackages/ui/src/lib/api/ProfileApi.tspackages/ui/src/lib/api/WizardApi.tspackages/ui/src/lib/api/index.tspackages/ui/src/lib/crypto/ai-key.tspackages/ui/src/lib/crypto/dek-login.tspackages/ui/src/lib/crypto/dek-session.ts
💤 Files with no reviewable changes (21)
- documentation/multi-watcher-plan.md
- packages/api/src/routes/prompt.routes.ts
- packages/api/src/routes/wizard.routes.ts
- packages/ui/src/lib/api/index.ts
- packages/api/src/routes/vault-watchers.routes.ts
- packages/api/src/routes/prompt/post-prompt.route.ts
- packages/api/src/routes/wizard/post-chat.route.ts
- packages/shared/src/contracts/ai.ts
- documentation/ai-watcher-plan.md
- packages/api/src/routes/prompt/get-usage.route.ts
- packages/ui/src/lib/api/WizardApi.ts
- packages/api/src/tests/wizard.service.test.ts
- packages/api/src/routes/vault-watchers/test-extract.route.ts
- packages/api/src/tests/gmail-ai-extractor.service.test.ts
- packages/api/src/services/gmail-ai-extractor.service.ts
- packages/api/src/services/wizard.service.ts
- packages/shared/src/contracts/wizard.ts
- packages/api/src/tests/prompt.service.test.ts
- packages/api/.env.example
- packages/api/package.json
- packages/api/src/services/prompt.service.ts
Comment on lines
+42
to
+47
| async insertIfNotExists(userId: string, fields: PendingGmailExpenseFields): Promise<PendingGmailExpense | null> { | ||
| const existing = await this.repo.findOne({ where: { userId, gmailMessageId: fields.gmailMessageId }, withDeleted: true }); | ||
| if (existing) return null; | ||
|
|
||
| const created = this.repo.create({ userId, ...fields }); | ||
| return this.repo.save(created); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files packages/api/src/repositories packages/api/src/entities packages/api/src/migrations | sed -n '1,200p'
printf '\n--- pending-gmail-expense.repository.ts ---\n'
cat -n packages/api/src/repositories/pending-gmail-expense.repository.ts
printf '\n--- user-ai-credential.repository.ts ---\n'
cat -n packages/api/src/repositories/user-ai-credential.repository.ts
printf '\n--- search for unique constraints / indexes ---\n'
rg -n "gmailMessageId|userId|unique|UNIQUE|`@Index`|`@Unique`|upsert|conflict" packages/api/src -g '!**/*.map'Repository: ali-ahnaf/pocket_pixel
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- PendingGmailExpense entity ---\n'
cat -n packages/api/src/entities/PendingGmailExpense.entity.ts
printf '\n--- UserAiCredential entity ---\n'
cat -n packages/api/src/entities/UserAiCredential.entity.ts
printf '\n--- related repository patterns ---\n'
sed -n '1,220p' packages/api/src/repositories/processed-gmail-message.repository.ts
sed -n '1,220p' packages/api/src/repositories/user-oauth-credential.repository.ts
printf '\n--- focused migration excerpts ---\n'
sed -n '1,120p' packages/api/src/migrations/1784885759673-AddPendingGmailExpenses.ts
sed -n '1,120p' packages/api/src/migrations/1784875736698-AddUserAiCredentials.tsRepository: ali-ahnaf/pocket_pixel
Length of output: 13749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "softDeleteForUser|softDelete\\(|user_ai_credentials|UserAiCredentialRepository|findByUserId\\(" packages/api/src -g '!**/*.map'
printf '\n--- entity/repository references for user-ai credential ---\n'
sed -n '1,220p' packages/api/src/entities/UserAiCredential.entity.ts
sed -n '1,220p' packages/api/src/repositories/user-ai-credential.repository.tsRepository: ali-ahnaf/pocket_pixel
Length of output: 13483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- pending-gmail-expense.service.ts ---\n'
sed -n '1,220p' packages/api/src/services/pending-gmail-expense.service.ts
printf '\n--- user-ai-credential.service.ts ---\n'
sed -n '1,220p' packages/api/src/services/user-ai-credential.service.tsRepository: ali-ahnaf/pocket_pixel
Length of output: 6080
Replace the read-before-write path with atomic conflict handling.
packages/api/src/repositories/pending-gmail-expense.repository.ts#L42-L47: this still has a TOCTOU window; use an insert-on-conflict path that returnsnullfor duplicates while preserving the intentionalwithDeleted: truereplay rule.packages/api/src/repositories/user-ai-credential.repository.ts#L37-L45: replacefindByUserId+savewith a DB-level upsert keyed byuserIdto avoid concurrent duplicate-row failures.
📍 Affects 2 files
packages/api/src/repositories/pending-gmail-expense.repository.ts#L42-L47(this comment)packages/api/src/repositories/user-ai-credential.repository.ts#L37-L45
🤖 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 `@packages/api/src/repositories/pending-gmail-expense.repository.ts` around
lines 42 - 47, The read-before-write flow in insertIfNotExists must become
atomic: use a database insert-on-conflict operation that returns null for
duplicate userId/gmailMessageId records while preserving the withDeleted: true
replay behavior; update
packages/api/src/repositories/pending-gmail-expense.repository.ts lines 42-47.
Replace findByUserId followed by save in
packages/api/src/repositories/user-ai-credential.repository.ts lines 37-45 with
a database-level upsert keyed by userId.
|
|
||
| const setAiCredentialSchema = Joi.object<SetAiCredentialInput>({ | ||
| salt: Joi.string().min(1).max(500).required(), | ||
| kdfIterations: Joi.number().integer().min(1).required(), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bound the PBKDF2 work factor at the API boundary.
kdfIterations: 1 is accepted and later returned as the browser’s PBKDF2 parameter, undermining resistance to offline password guessing. An excessively large value can also persist a client-side CPU denial of service. Enforce a shared, versioned minimum and maximum (or supported fixed value) before storing it.
🤖 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 `@packages/api/src/routes/ai-credentials/put-ai-credentials.route.ts` at line
11, Update the kdfIterations validation in the AI credentials request schema to
enforce the shared, versioned PBKDF2 minimum and maximum (or supported fixed
value) instead of only min(1). Reuse the existing shared constants or
configuration symbols, and ensure the bounded value is validated before it can
be stored or returned to the browser.
| date: Joi.string() | ||
| .pattern(/^\d{4}-\d{2}-\d{2}$/) | ||
| .optional(), | ||
| isCommitted: Joi.boolean().optional(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate isCommitted through the shared client contract.
Adding the field only to server validation leaves ProfileApi.createTransaction’s payload type without isCommitted, so the pending-review flow cannot submit it through the standard API client. Update CreateTransactionInput and have ProfileApi.createTransaction consume that shared DTO.
As per coding guidelines, request and response DTOs must be defined in the shared contracts package and not redefined locally in the API or UI.
🤖 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 `@packages/api/src/routes/transactions/post-transaction.route.ts` at line 17,
Propagate isCommitted through the shared transaction contract: add it to
CreateTransactionInput in the shared contracts package, then update
ProfileApi.createTransaction to accept and use that shared DTO instead of a
locally defined payload type. Ensure the API and UI do not redeclare the request
DTO.
Source: Coding guidelines
|
|
||
| try { | ||
| await profileApi.changePassword(user.id, { currentPassword, newPassword }); | ||
| // TODO(T4/T8): re-derive KEK from newPassword, re-wrap the DEK (@/lib/crypto/ai-key), and PUT the updated wrappedDek/salt once the ai-credentials endpoint exists. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Re-wrap the AI credential before reporting success.
The TODO is stale because the ai-credentials API now exists. After a password change, leaving wrappedDek protected by the old password-derived KEK makes the saved OpenRouter key undecryptable under the new password. Perform the rewrap in a recoverable flow and surface failures instead of showing success.
🤖 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 `@packages/ui/src/app/change-password/page.tsx` at line 50, The password-change
flow must re-wrap the AI credential before reporting success. Replace the stale
TODO near the password-change handler with a recoverable sequence that derives a
KEK from the new password, re-wraps the DEK using the existing crypto helpers,
and updates wrappedDek and salt through the ai-credentials endpoint; surface any
failure and do not show success when rewrapping or persistence fails.
Comment on lines
+17
to
+20
| function isCuratedMatch(modelId: string): boolean { | ||
| const lowerId = modelId.toLowerCase(); | ||
| return CURATED_MODEL_IDS.some((curatedId) => lowerId.includes(curatedId) || curatedId.includes(lowerId)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo '--- target file ---'
cat -n packages/ui/src/app/settings/ai/ModelPicker.tsx | sed -n '1,220p'
echo '--- search CURATED_MODEL_IDS ---'
rg -n "CURATED_MODEL_IDS|isCuratedMatch|toLowerCase\(\)\.includes|includes\(lowerId\)" packages/ui/src/app/settings -SRepository: ali-ahnaf/pocket_pixel
Length of output: 6021
🌐 Web query:
OpenRouter docs openai/gpt-4:free model alias openai/gpt-4 distinct model id
💡 Result:
openai/gpt-4:free is not a separate base model alias; it’s the openai/gpt-4 model with the :free variant suffix. OpenRouter docs say to “append :free to any model ID,” and that single-model lookup supports variant suffixes and resolves aliases automatically. (openrouter.ai)
So: same model family, different variant, not a totally distinct model ID in the usual sense. (openrouter.ai)
Citations:
- 1: https://openrouter.ai/docs/guides/routing/model-variants/free
- 2: https://openrouter.ai/docs/guides/overview/models
Remove the reverse substring check.
curatedId.includes(lowerId) lets a shorter, uncurated id like openai/gpt-4 match the curated openai/gpt-4o, so it can slip into the curated picker. Reuse isCuratedMatch in the useEffect warning too so the two paths stay aligned.
🤖 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 `@packages/ui/src/app/settings/ai/ModelPicker.tsx` around lines 17 - 20, Update
isCuratedMatch to match only when the model ID contains a curated ID, removing
the reverse curatedId.includes check. In the useEffect warning path, replace its
separate matching logic with isCuratedMatch so picker filtering and warnings
remain consistent.
Comment on lines
+106
to
+116
| await profileApi.createTransaction(userId, { | ||
| amount: parsed.amount, | ||
| type: parsed.type, | ||
| tagIds: parsed.tagIds, | ||
| title: parsed.title, | ||
| vaultId: item.vaultId, | ||
| date: parsed.date, | ||
| }); | ||
| await profileApi.deletePendingExpense(userId, item.id); | ||
| onResolved(item.id); | ||
| } catch (err) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Duplicate transaction risk if deletePendingExpense fails after createTransaction succeeds.
If the transaction create succeeds but the subsequent delete throws, onResolved is never called — the item stays selected and re-clickable, so a retry re-runs createTransaction and produces a duplicate expense. These two calls should be reconciled as a single atomic operation (ideally one backend endpoint that creates-and-resolves), or the client should track that a transaction was already created for this item and only retry the delete on subsequent attempts.
🤖 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 `@packages/ui/src/components/pending-expenses/PendingExpenseDetailModal.tsx`
around lines 106 - 116, The resolve flow around createTransaction and
deletePendingExpense is not retry-safe: a successful transaction creation
followed by delete failure can create duplicates. Reconcile these operations
through a single atomic backend create-and-resolve endpoint if available;
otherwise track the successful creation for item.id and, on retries, skip
createTransaction and retry only deletePendingExpense before calling onResolved.
Comment on lines
+27
to
+35
| const fetchPending = useCallback((): void => { | ||
| if (!userId) return; | ||
| Promise.all([profileApi.getPendingExpenses(userId), profileApi.getTags(userId)]) | ||
| .then(([pending, tagList]) => { | ||
| setItems(pending); | ||
| setTags(tagList); | ||
| }) | ||
| .catch((err) => setError(profileApi.parseError(err))); | ||
| }, [userId]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale error not cleared on successful re-fetch.
fetchPending never resets error before/after a successful fetch, so if an earlier attempt failed, a later successful re-fetch (e.g. userId populated after auth resolves) still shows the old error banner alongside the newly loaded items.
🐛 Proposed fix
const fetchPending = useCallback((): void => {
if (!userId) return;
Promise.all([profileApi.getPendingExpenses(userId), profileApi.getTags(userId)])
.then(([pending, tagList]) => {
setItems(pending);
setTags(tagList);
+ setError(null);
})
.catch((err) => setError(profileApi.parseError(err)));
}, [userId]);📝 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
| const fetchPending = useCallback((): void => { | |
| if (!userId) return; | |
| Promise.all([profileApi.getPendingExpenses(userId), profileApi.getTags(userId)]) | |
| .then(([pending, tagList]) => { | |
| setItems(pending); | |
| setTags(tagList); | |
| }) | |
| .catch((err) => setError(profileApi.parseError(err))); | |
| }, [userId]); | |
| const fetchPending = useCallback((): void => { | |
| if (!userId) return; | |
| Promise.all([profileApi.getPendingExpenses(userId), profileApi.getTags(userId)]) | |
| .then(([pending, tagList]) => { | |
| setItems(pending); | |
| setTags(tagList); | |
| setError(null); | |
| }) | |
| .catch((err) => setError(profileApi.parseError(err))); | |
| }, [userId]); |
🤖 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 `@packages/ui/src/components/pending-expenses/PendingExpensesPanel.tsx` around
lines 27 - 35, Update fetchPending to clear the existing error when a fetch
succeeds, ensuring a later successful re-fetch removes any stale error banner
while preserving the current item and tag updates and error handling.
Comment on lines
+99
to
+153
| export async function chat({ apiKey, model, messages, responseFormat }: ChatParams): Promise<string> { | ||
| const response = await fetch(CHAT_COMPLETIONS_URL, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${apiKey}`, | ||
| 'Content-Type': 'application/json', | ||
| 'HTTP-Referer': APP_REFERER, | ||
| 'X-Title': APP_TITLE, | ||
| }, | ||
| body: JSON.stringify({ | ||
| model, | ||
| messages, | ||
| ...(responseFormat ? { response_format: responseFormat } : {}), | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| await throwOpenRouterError(response); | ||
| } | ||
|
|
||
| const data: OpenRouterChatCompletionResponse = await response.json(); | ||
| const content = data.choices?.[0]?.message?.content; | ||
|
|
||
| if (!content) { | ||
| throw new Error('OpenRouter response did not include any message content'); | ||
| } | ||
|
|
||
| return content; | ||
| } | ||
|
|
||
| /** | ||
| * Fetch the list of models available on OpenRouter. Public endpoint — no | ||
| * Authorization header required. | ||
| */ | ||
| export async function listModels(): Promise<OpenRouterModel[]> { | ||
| const response = await fetch(MODELS_URL, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'HTTP-Referer': APP_REFERER, | ||
| 'X-Title': APP_TITLE, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| await throwOpenRouterError(response); | ||
| } | ||
|
|
||
| const data: OpenRouterModelListResponse = await response.json(); | ||
|
|
||
| return data.data.map((entry) => ({ | ||
| id: entry.id, | ||
| name: entry.name || entry.id, | ||
| contextLength: entry.context_length, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a request timeout to chat()/listModels().
Neither fetch call has a timeout. A hung/slow OpenRouter response leaves the wizard chat, pending-expense parse action, and the watcher dry-run modal stuck in a perpetual loading state with no recovery short of a page reload.
⏱️ Proposed fix: bound both requests with an AbortController timeout
+const DEFAULT_TIMEOUT_MS = 60_000;
+
+function withTimeout(timeoutMs: number): { signal: AbortSignal; cancel: () => void } {
+ const controller = new AbortController();
+ const id = setTimeout(() => controller.abort(), timeoutMs);
+ return { signal: controller.signal, cancel: () => clearTimeout(id) };
+}
+
export async function chat({ apiKey, model, messages, responseFormat }: ChatParams): Promise<string> {
+ const { signal, cancel } = withTimeout(DEFAULT_TIMEOUT_MS);
const response = await fetch(CHAT_COMPLETIONS_URL, {
method: 'POST',
+ signal,
headers: {
Authorization: `Bearer ${apiKey}`,
'Content-Type': 'application/json',
'HTTP-Referer': APP_REFERER,
'X-Title': APP_TITLE,
},
body: JSON.stringify({
model,
messages,
...(responseFormat ? { response_format: responseFormat } : {}),
}),
- });
+ }).finally(cancel);🤖 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 `@packages/ui/src/lib/ai/openrouter.ts` around lines 99 - 153, Add an
AbortController-based timeout to both fetch calls in chat() and listModels(),
passing each controller’s signal to fetch and aborting after the configured
timeout. Ensure the timeout is cleared when each request completes, while
preserving existing response parsing and error handling.
Comment on lines
+130
to
+151
| export async function getSessionDek(): Promise<CryptoKey | null> { | ||
| if (hydrated) return currentDek; | ||
| if (!hydrationPromise) { | ||
| hydrationPromise = (async () => { | ||
| if (typeof window === 'undefined') { | ||
| hydrated = true; | ||
| return null; | ||
| } | ||
| const stored = window.sessionStorage.getItem(SESSION_STORAGE_KEY); | ||
| if (!stored) { | ||
| hydrated = true; | ||
| return null; | ||
| } | ||
| const dek = await importRawDek(base64ToBuffer(stored)); | ||
| currentDek = dek; | ||
| hydrated = true; | ||
| notify(); | ||
| return dek; | ||
| })(); | ||
| } | ||
| return hydrationPromise; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Hydration failure permanently wedges getSessionDek().
If importRawDek rejects (corrupted/invalid bytes in sessionStorage), hydrated never gets set to true and hydrationPromise is never cleared — every subsequent call to getSessionDek() for the rest of the page session returns the same rejected promise. Downstream, useDekSession.ts's getSessionDek().finally(...) (no .catch) turns this into an unhandled promise rejection on every mount, with loading never settling correctly and the DEK never recoverable without a hard reload.
🔧 Proposed fix: recover from hydration failure
const stored = window.sessionStorage.getItem(SESSION_STORAGE_KEY);
if (!stored) {
hydrated = true;
return null;
}
- const dek = await importRawDek(base64ToBuffer(stored));
- currentDek = dek;
- hydrated = true;
- notify();
- return dek;
+ try {
+ const dek = await importRawDek(base64ToBuffer(stored));
+ currentDek = dek;
+ return dek;
+ } catch {
+ window.sessionStorage.removeItem(SESSION_STORAGE_KEY);
+ currentDek = null;
+ return null;
+ } finally {
+ hydrated = true;
+ notify();
+ }📝 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
| export async function getSessionDek(): Promise<CryptoKey | null> { | |
| if (hydrated) return currentDek; | |
| if (!hydrationPromise) { | |
| hydrationPromise = (async () => { | |
| if (typeof window === 'undefined') { | |
| hydrated = true; | |
| return null; | |
| } | |
| const stored = window.sessionStorage.getItem(SESSION_STORAGE_KEY); | |
| if (!stored) { | |
| hydrated = true; | |
| return null; | |
| } | |
| const dek = await importRawDek(base64ToBuffer(stored)); | |
| currentDek = dek; | |
| hydrated = true; | |
| notify(); | |
| return dek; | |
| })(); | |
| } | |
| return hydrationPromise; | |
| } | |
| export async function getSessionDek(): Promise<CryptoKey | null> { | |
| if (hydrated) return currentDek; | |
| if (!hydrationPromise) { | |
| hydrationPromise = (async () => { | |
| if (typeof window === 'undefined') { | |
| hydrated = true; | |
| return null; | |
| } | |
| const stored = window.sessionStorage.getItem(SESSION_STORAGE_KEY); | |
| if (!stored) { | |
| hydrated = true; | |
| return null; | |
| } | |
| try { | |
| const dek = await importRawDek(base64ToBuffer(stored)); | |
| currentDek = dek; | |
| return dek; | |
| } catch { | |
| window.sessionStorage.removeItem(SESSION_STORAGE_KEY); | |
| currentDek = null; | |
| return null; | |
| } finally { | |
| hydrated = true; | |
| notify(); | |
| } | |
| })(); | |
| } | |
| return hydrationPromise; | |
| } |
🤖 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 `@packages/ui/src/lib/crypto/dek-session.ts` around lines 130 - 151, Update
getSessionDek so hydration failures from importRawDek do not permanently retain
a rejected hydrationPromise: clear hydrationPromise in the failure path, reset
or preserve hydration state so a later call can retry, and ensure the rejected
attempt is propagated to its caller. Keep successful hydration behavior,
including currentDek assignment and notify(), unchanged.
| 3. You get a **push notification**, open the pending item in the UI, and it's parsed **client-side**, in your browser, using your own OpenRouter API key. | ||
| 4. Confirm and it becomes a transaction; dismiss and it's cleared from the queue. | ||
|
|
||
| The **Wizard Assistant** chat (Settings → AI) uses the same client-side OpenRouter key to answer questions about your spending — nothing is sent to Pocket Pixel's own servers for either feature. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Correct the privacy statement for the pending-email flow.
getPendingExpenseEmail retrieves the email body through Pocket Pixel’s API, and confirmed transactions are also sent through the transaction API. “Nothing is sent to Pocket Pixel’s own servers for either feature” is therefore misleading; limit the claim to OpenRouter key plaintext and AI-processing requests.
🤖 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 `@README.md` at line 233, Update the privacy statement in the Wizard Assistant
description to remove the claim that nothing is sent to Pocket Pixel’s servers,
since getPendingExpenseEmail and confirmed transactions use Pocket Pixel APIs.
Limit the claim to the client-side OpenRouter key and AI-processing requests,
while preserving the existing feature description.
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit