feat: implement offline-first synchronization support using an idempo… - #288
Open
ali-ahnaf wants to merge 1 commit into
Open
feat: implement offline-first synchronization support using an idempo…#288ali-ahnaf wants to merge 1 commit into
ali-ahnaf wants to merge 1 commit into
Conversation
…tent local outbox for debts and transactions.
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. |
📝 WalkthroughWalkthroughChangesOffline support is added for transaction and debt creation through local caching, a FIFO outbox, reconnect replay, queued placeholders, offline status UI, service-worker shell precaching, and Offline support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Outbox
participant Sync
participant API
participant Database
User->>UI: Create transaction or debt offline
UI->>Outbox: Store queued operation
Sync->>Outbox: Read queued operations on reconnect
Sync->>API: Replay create with clientRequestId
API->>Database: Find existing or insert record
Database-->>API: Return record
API-->>Sync: Return create result
Sync->>Outbox: Remove completed operation
Sync-->>UI: Dispatch refresh event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/services/transactions.service.ts (1)
52-69: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the transaction and tag links atomically.
If
save()succeeds butreplaceTags()fails, a retry with this key returns the existing expense at Line 56 and permanently skips its requested tags. Wrap creation and tag replacement in one transaction, or make replay complete incomplete tag links.🤖 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/services/transactions.service.ts` around lines 52 - 69, Update the transaction creation flow around createEntity, save, and replaceTags so transaction persistence and tag-link replacement occur atomically, rolling back both when either operation fails. Ensure clientRequestId replay handling does not return an existing transaction until its requested tag links are complete, preserving the existing replay behavior for fully persisted transactions.
🤖 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 `@documentation/offline-support.md`:
- Around line 10-21: Update the offline-support document’s transaction-creation
section to reflect the current LogResourceModal.handleSubmit behavior: transport
failures are caught and the write is queued rather than producing an unhandled
rejection. Preserve the remaining offline limitations and identify the former
no-catch behavior as historical context only if it remains useful.
In
`@packages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts`:
- Around line 14-25: Scope client request ID uniqueness to each user instead of
globally: in
packages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts
lines 14-25, replace both single-column unique constraints with composite
constraints on userId and clientRequestId; in
packages/api/src/entities/Debt.entity.ts lines 32-37 and
packages/api/src/entities/Expense.entity.ts lines 46-51, remove unique: true
from clientRequestId and declare matching composite unique indexes.
In `@packages/api/src/services/debts.service.ts`:
- Around line 38-66: Make the lookup-and-create flow atomic in
packages/api/src/services/debts.service.ts:38-66 around the debt creation
method, using insert-on-conflict or catching the unique-key violation and
re-reading the persisted debt so concurrent replays return the winner. Apply the
same atomic handling to packages/api/src/services/transactions.service.ts:50-67
around its transaction creation flow, preserving existing response mapping and
replay behavior.
In `@packages/api/src/tests/debts.service.test.ts`:
- Around line 170-199: The DebtsService.create path must handle concurrent
clientRequestId races: if debts.save fails after the initial lookup, reload the
debt with debts.findOneByClientRequestId for the same user and return it when
found, while preserving normal error propagation when no existing debt is found.
Add a regression test covering two concurrent creates that miss the initial
lookup, one encounters the unique-constraint save failure, and the failed
request returns the persisted existing debt.
In `@packages/ui/src/app/debts/page.tsx`:
- Around line 88-124: Update handleCreate to accept the shared CreateDebtInput
type and generate one client request ID before the online attempt. Include that
ID in profileApi.createDebt and pass the same ID to enqueue when a NetworkError
triggers queueLocally, ensuring replay reuses the original request identity
instead of creating a new one.
In `@packages/ui/src/app/layout.tsx`:
- Line 6: Update the OfflineSync import in the layout module to use the named
export provided by OfflineSync.tsx, while preserving the existing component
usage.
In `@packages/ui/src/components/OfflineSync.tsx`:
- Around line 33-39: The offline synchronization flow must use the authenticated
batch sync contract rather than replaying direct create requests. In
packages/ui/src/components/OfflineSync.tsx lines 33-39, submit queued typed
operations to POST /api/users/:userId/sync and process each returned operation
result, including per-operation failures. In packages/ui/e2e/offline.spec.ts
lines 88-94 and 123-128, update the transaction and debt scenarios to assert
each operation is included in one sync request instead of asserting direct
transaction or debt POSTs. In documentation/offline-support.md lines 92-100,
describe the batch endpoint and its per-operation failure behavior.
- Around line 42-57: Update the sync flow in OfflineSync so OFFLINE_SYNCED_EVENT
is dispatched when entries are successfully synchronized or terminally removed.
Track whether any queue entry was removed in the non-NetworkError path and use
that alongside synced when deciding to dispatch the event, while preserving the
existing retry and ordering behavior.
In `@packages/ui/src/lib/api/ApiClient.ts`:
- Around line 100-109: Update the enqueue() transaction flow to generate and
persist a stable clientRequestId before the initial POST, including when the
request reaches the server but loses its response. Ensure the NetworkError
retry/replay path reuses that persisted id instead of creating a new one,
preventing duplicate rows.
In `@packages/ui/src/lib/offline/outbox.ts`:
- Around line 49-50: Update the persistence flow surrounding the localStorage
write in the outbox module to protect queued financial entries using the
existing session DEK or equivalent encrypted persistence before storage. Ensure
debt titles, amounts, notes, dates, and transaction details are never written as
plaintext under OUTBOX_STORAGE_KEY, while preserving the existing outbox queue
behavior and error handling.
- Around line 49-76: The enqueue flow must not report success when localStorage
persistence fails. Update write and enqueue so setItem errors propagate as a
storage failure, allowing callers to keep the form open and show an actionable
error; preserve successful queueing behavior and add coverage for the
persistence-failure path.
---
Outside diff comments:
In `@packages/api/src/services/transactions.service.ts`:
- Around line 52-69: Update the transaction creation flow around createEntity,
save, and replaceTags so transaction persistence and tag-link replacement occur
atomically, rolling back both when either operation fails. Ensure
clientRequestId replay handling does not return an existing transaction until
its requested tag links are complete, preserving the existing replay behavior
for fully persisted transactions.
🪄 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: fabccbc7-b36b-4bcb-a7c0-06b89d32740b
📒 Files selected for processing (35)
documentation/offline-support.mdpackages/api/src/entities/Debt.entity.tspackages/api/src/entities/Expense.entity.tspackages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.tspackages/api/src/repositories/debts.repository.tspackages/api/src/repositories/transactions.repository.tspackages/api/src/routes/debts/post-debt.route.tspackages/api/src/routes/transactions/post-transaction.route.tspackages/api/src/services/debts.service.tspackages/api/src/services/transactions.service.tspackages/api/src/tests/debts.service.test.tspackages/api/src/tests/transactions.service.test.tspackages/shared/src/contracts/debts.tspackages/shared/src/contracts/transactions.tspackages/ui/e2e/offline.spec.tspackages/ui/public/sw.jspackages/ui/src/app/debts/page.tsxpackages/ui/src/app/layout.tsxpackages/ui/src/app/page.tsxpackages/ui/src/components/AppBar.tsxpackages/ui/src/components/LogResourceModal.test.tsxpackages/ui/src/components/LogResourceModal.tsxpackages/ui/src/components/OfflineSync.tsxpackages/ui/src/components/index.tspackages/ui/src/hooks/useAuth.tspackages/ui/src/hooks/useDisplaySettings.tspackages/ui/src/hooks/useOnlineStatus.tspackages/ui/src/lib/api/ApiClient.test.tspackages/ui/src/lib/api/ApiClient.tspackages/ui/src/lib/api/ProfileApi.tspackages/ui/src/lib/offline/cache.test.tspackages/ui/src/lib/offline/cache.tspackages/ui/src/lib/offline/outbox.test.tspackages/ui/src/lib/offline/outbox.tspackages/ui/src/lib/offline/sync-events.ts
Comment on lines
+10
to
+21
| ## 1. Does creating a transaction work offline today? No. | ||
|
|
||
| [`handleSubmit`](../packages/ui/src/components/LogResourceModal.tsx#L244-L268) calls `profileApi.createTransaction`, axios rejects with a transport error, and there is **no `catch`** — only `try/finally`. So offline the user sees: spinner flips back to `RECORD`, modal stays open, fields keep their values, nothing saved, an unhandled promise rejection in the console. No error message at all. | ||
|
|
||
| Two more things break in that modal before submit even happens: | ||
|
|
||
| - On open it fires `getTags` and `getVaults` ([lines 81-92](../packages/ui/src/components/LogResourceModal.tsx#L81-L92)) with `.then()` and no `.catch()`. Offline → both reject → tag list empty, `vaults.length === 0` so the vault picker is not rendered at all, and any submit would post `vaultId: null`. | ||
| - The AI prompt path calls `getAiCredentialStatus` and then OpenRouter — both need the network. | ||
|
|
||
| Debts are simpler: `AddDebtModal` is pure UI with no fetching, and `handleCreate` ([debts/page.tsx:74-78](../packages/ui/src/app/debts/page.tsx#L74-L78)) needs the server's returned `DebtDto` only to prepend it to the list — a value we can construct locally. | ||
|
|
||
| Also relevant: the service worker precaches only `/` ([sw.js](../packages/ui/public/sw.js)), so a cold offline launch straight into `/debts/` renders the wrong shell. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the document to describe the shipped behavior.
This says offline transaction creation fails with no catch, but LogResourceModal.tsx now catches transport failures and queues the write. Mark this as historical context or rewrite it as current behavior.
🤖 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 `@documentation/offline-support.md` around lines 10 - 21, Update the
offline-support document’s transaction-creation section to reflect the current
LogResourceModal.handleSubmit behavior: transport failures are caught and the
write is queued rather than producing an unhandled rejection. Preserve the
remaining offline limitations and identify the former no-catch behavior as
historical context only if it remains useful.
Comment on lines
+14
to
+25
| `CREATE TABLE "temporary_expenses" ("id" varchar PRIMARY KEY NOT NULL, "userId" varchar NOT NULL, "title" varchar(200), "amount" decimal(10,2) NOT NULL DEFAULT (0), "type" varchar NOT NULL DEFAULT ('expense'), "date" date, "interval" varchar, "startDate" date, "endDate" date, "deletedAt" datetime, "vaultId" varchar, "sourceRecurringId" varchar, "createdAt" datetime DEFAULT (datetime('now')), "updatedAt" datetime DEFAULT (datetime('now')), "clientRequestId" varchar, CONSTRAINT "UQ_f833020de04e8bcfbf00ac86cfe" UNIQUE ("clientRequestId"), CONSTRAINT "FK_3d211de716f0f14ea7a8a4b1f2c" FOREIGN KEY ("userId") REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, CONSTRAINT "FK_e0d76d9858620c98a05e7785ef7" FOREIGN KEY ("vaultId") REFERENCES "vaults" ("id") ON DELETE SET NULL ON UPDATE NO ACTION)`, | ||
| ); | ||
| await queryRunner.query( | ||
| `INSERT INTO "temporary_expenses"("id", "userId", "title", "amount", "type", "date", "interval", "startDate", "endDate", "deletedAt", "vaultId", "sourceRecurringId", "createdAt", "updatedAt") SELECT "id", "userId", "title", "amount", "type", "date", "interval", "startDate", "endDate", "deletedAt", "vaultId", "sourceRecurringId", "createdAt", "updatedAt" FROM "expenses"`, | ||
| ); | ||
| await queryRunner.query(`DROP TABLE "expenses"`); | ||
| await queryRunner.query(`ALTER TABLE "temporary_expenses" RENAME TO "expenses"`); | ||
| await queryRunner.query( | ||
| `CREATE TABLE "temporary_debts" ("id" varchar PRIMARY KEY NOT NULL, "userId" varchar NOT NULL, "title" varchar(200) NOT NULL, "amount" decimal(10,2) NOT NULL DEFAULT (0), "type" varchar NOT NULL DEFAULT ('expense'), "createdAt" datetime DEFAULT (datetime('now')), "updatedAt" datetime DEFAULT (datetime('now')), "deletedAt" datetime, "notes" text, "completed" boolean NOT NULL DEFAULT (0), "dueDate" date, "clientRequestId" varchar, CONSTRAINT "UQ_14272a37ffc2934919cb0d2fe1b" UNIQUE ("clientRequestId"), CONSTRAINT "FK_834960a509c776eb841644a9bac" FOREIGN KEY ("userId") REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION)`, | ||
| ); | ||
| await queryRunner.query( | ||
| `INSERT INTO "temporary_debts"("id", "userId", "title", "amount", "type", "createdAt", "updatedAt", "deletedAt", "notes", "completed", "dueDate") SELECT "id", "userId", "title", "amount", "type", "createdAt", "updatedAt", "deletedAt", "notes", "completed", "dueDate" FROM "debts"`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope idempotency uniqueness to the user. Repository lookups use (userId, clientRequestId), but these constraints make clientRequestId globally unique. A key used by another user therefore raises a constraint error instead of creating that user’s independent request.
packages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts#L14-L25: replace each single-column unique constraint with a composite unique constraint on("userId", "clientRequestId").packages/api/src/entities/Debt.entity.ts#L32-L37: removeunique: truefrom the column and declare the matching composite unique index.packages/api/src/entities/Expense.entity.ts#L46-L51: removeunique: truefrom the column and declare the matching composite unique index.
📍 Affects 3 files
packages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts#L14-L25(this comment)packages/api/src/entities/Debt.entity.ts#L32-L37packages/api/src/entities/Expense.entity.ts#L46-L51
🤖 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/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts`
around lines 14 - 25, Scope client request ID uniqueness to each user instead of
globally: in
packages/api/src/migrations/1785085924293-AddClientRequestIdToExpensesAndDebts.ts
lines 14-25, replace both single-column unique constraints with composite
constraints on userId and clientRequestId; in
packages/api/src/entities/Debt.entity.ts lines 32-37 and
packages/api/src/entities/Expense.entity.ts lines 46-51, remove unique: true
from clientRequestId and declare matching composite unique indexes.
Comment on lines
+38
to
66
| // A create queued while the client was offline may already have reached the | ||
| // server on its first attempt; return that row rather than inserting a twin. | ||
| if (input.clientRequestId) { | ||
| const existing = await this.debts.findOneByClientRequestId(userId, input.clientRequestId); | ||
| if (existing) { | ||
| logger.info('Returned existing debt for replayed clientRequestId', { userId, debtId: existing.id }); | ||
| return { | ||
| id: existing.id, | ||
| userId: existing.userId, | ||
| title: existing.title, | ||
| amount: Number(existing.amount), | ||
| type: existing.type, | ||
| notes: existing.notes ?? null, | ||
| dueDate: existing.dueDate ?? null, | ||
| createdAt: existing.createdAt, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const debt = this.debts.createEntity({ | ||
| userId, | ||
| title: input.title, | ||
| amount: input.amount, | ||
| type: input.type, | ||
| notes: input.notes ?? null, | ||
| dueDate: input.dueDate ?? null, | ||
| clientRequestId: input.clientRequestId ?? null, | ||
| }); | ||
| const saved = await this.debts.save(debt); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make replay handling atomic. Two simultaneous retries can both miss the initial lookup, then one insert fails on the unique key rather than returning the persisted record. Use an insert-on-conflict strategy, or catch the unique violation and re-read the winner.
packages/api/src/services/debts.service.ts#L38-L66: make the debt lookup-and-create sequence atomic.packages/api/src/services/transactions.service.ts#L50-L67: make the transaction lookup-and-create sequence atomic.
📍 Affects 2 files
packages/api/src/services/debts.service.ts#L38-L66(this comment)packages/api/src/services/transactions.service.ts#L50-L67
🤖 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/services/debts.service.ts` around lines 38 - 66, Make the
lookup-and-create flow atomic in
packages/api/src/services/debts.service.ts:38-66 around the debt creation
method, using insert-on-conflict or catching the unique-key violation and
re-reading the persisted debt so concurrent replays return the winner. Apply the
same atomic handling to packages/api/src/services/transactions.service.ts:50-67
around its transaction creation flow, preserving existing response mapping and
replay behavior.
Comment on lines
+170
to
+199
| it('persists the clientRequestId of an offline-queued create', async () => { | ||
| const saved = buildDebt(); | ||
| debts.findOneByClientRequestId.mockResolvedValue(null); | ||
| debts.save.mockResolvedValue(saved); | ||
|
|
||
| await service.create('user-1', { ...input, clientRequestId: 'b3f1c2d4-0000-4000-8000-000000000002' }); | ||
|
|
||
| expect(debts.createEntity).toHaveBeenCalledWith(expect.objectContaining({ clientRequestId: 'b3f1c2d4-0000-4000-8000-000000000002' })); | ||
| }); | ||
|
|
||
| it('returns the existing debt instead of inserting a duplicate when the clientRequestId was already used', async () => { | ||
| const existing = buildDebt({ id: 'debt-existing' }); | ||
| debts.findOneByClientRequestId.mockResolvedValue(existing); | ||
|
|
||
| const result = await service.create('user-1', { ...input, clientRequestId: 'b3f1c2d4-0000-4000-8000-000000000002' }); | ||
|
|
||
| expect(debts.findOneByClientRequestId).toHaveBeenCalledWith('user-1', 'b3f1c2d4-0000-4000-8000-000000000002'); | ||
| expect(debts.createEntity).not.toHaveBeenCalled(); | ||
| expect(debts.save).not.toHaveBeenCalled(); | ||
| expect(result).toMatchObject({ id: 'debt-existing', userId: existing.userId, title: existing.title }); | ||
| }); | ||
|
|
||
| it('does not look up an idempotency key when the create carries none', async () => { | ||
| debts.save.mockResolvedValue(buildDebt()); | ||
|
|
||
| await service.create('user-1', input); | ||
|
|
||
| expect(debts.findOneByClientRequestId).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files
git ls-files 'packages/api/src/**/debts*' 'packages/api/src/**/debt*' 'packages/api/src/**/repositories/**' 'packages/api/src/**/services/**' 'packages/api/src/tests/**' | sed 's#^`#FILE` #'
echo '--- debts.service.test.ts ---'
cat -n packages/api/src/tests/debts.service.test.ts | sed -n '1,260p'
echo '--- search clientRequestId / duplicate key ---'
rg -n "clientRequestId|duplicate|unique constraint|23505|E11000|save\\(|findOneByClientRequestId|create\\(" packages/api/src --glob '!**/dist/**' --glob '!**/build/**'Repository: ali-ahnaf/pocket_pixel
Length of output: 34816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files
git ls-files 'packages/api/src/**/debts*' 'packages/api/src/src/**' 'packages/api/src/**/repositories/**' 'packages/api/src/**/services/**' 'packages/api/src/tests/**' | sed 's#^`#FILE` #'
echo '--- debts.service.test.ts ---'
cat -n packages/api/src/tests/debts.service.test.ts | sed -n '1,260p'
echo '--- search clientRequestId / duplicate key ---'
rg -n "clientRequestId|duplicate|unique constraint|23505|E11000|save\\(|findOneByClientRequestId|create\\(" packages/api/src --glob '!**/dist/**' --glob '!**/build/**'Repository: ali-ahnaf/pocket_pixel
Length of output: 34816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- debts.service.test.ts ---'
cat -n packages/api/src/tests/debts.service.test.ts | sed -n '140,230p'
echo '--- debts service sources ---'
git ls-files 'packages/api/src/**/debts*.ts' 'packages/api/src/**/repositories/*.ts' 'packages/api/src/**/services/*.ts' | sed 's#^`#FILE` #'
echo '--- search clientRequestId / duplicate key handling ---'
rg -n "clientRequestId|findOneByClientRequestId|duplicate key|unique constraint|23505|E11000|QueryFailedError|createEntity|save\\(" packages/api/src --glob '!**/dist/**' --glob '!**/build/**'Repository: ali-ahnaf/pocket_pixel
Length of output: 30272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- debts.service.ts ---'
cat -n packages/api/src/services/debts.service.ts | sed -n '1,120p'
echo '--- transactions.service.ts (parallel idempotency pattern) ---'
cat -n packages/api/src/services/transactions.service.ts | sed -n '1,100p'
echo '--- debt entity unique constraint ---'
cat -n packages/api/src/entities/Debt.entity.ts | sed -n '1,90p'Repository: ali-ahnaf/pocket_pixel
Length of output: 11680
Handle duplicate-key races in DebtsService.create. Two concurrent replays can both miss the lookup and one will fail on the unique clientRequestId constraint; catch that save error, reload the existing debt for the same user, and add a concurrent regression test.
🤖 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/tests/debts.service.test.ts` around lines 170 - 199, The
DebtsService.create path must handle concurrent clientRequestId races: if
debts.save fails after the initial lookup, reload the debt with
debts.findOneByClientRequestId for the same user and return it when found, while
preserving normal error propagation when no existing debt is found. Add a
regression test covering two concurrent creates that miss the initial lookup,
one encounters the unique-constraint save failure, and the failed request
returns the persisted existing debt.
Comment on lines
88
to
+124
| const handleCreate = async (data: { title: string; amount: number; type: 'expense' | 'income'; notes: string | null; dueDate: string | null }) => { | ||
| if (!userId) return; | ||
| const created = await profileApi.createDebt(userId, data); | ||
| setDebts((prev) => [created, ...prev]); | ||
|
|
||
| // Every field of a due is known client-side, so an offline create can render | ||
| // exactly like an online one — only the id marks it as not-yet-synced. | ||
| const queueLocally = () => { | ||
| const entry = enqueue({ kind: 'create-debt', userId, payload: data }); | ||
| setDebts((prev) => [ | ||
| { | ||
| id: `${OFFLINE_ID_PREFIX}${entry.id}`, | ||
| userId, | ||
| title: data.title, | ||
| amount: data.amount, | ||
| type: data.type, | ||
| notes: data.notes, | ||
| dueDate: data.dueDate, | ||
| createdAt: new Date(), | ||
| completed: false, | ||
| discarded: false, | ||
| }, | ||
| ...prev, | ||
| ]); | ||
| }; | ||
|
|
||
| if (!navigator.onLine) { | ||
| queueLocally(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const created = await profileApi.createDebt(userId, data); | ||
| setDebts((prev) => [created, ...prev]); | ||
| } catch (err) { | ||
| // Only a transport failure is queued; a server rejection must surface. | ||
| if (!(err instanceof NetworkError)) throw err; | ||
| queueLocally(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use one client request ID for the initial attempt and replay.
Line 118 posts a payload without clientRequestId. If the server commits but its response is lost, the fallback enqueues a new ID and replay inserts a duplicate. Generate the ID before the first POST, send it in that request, and preserve it when enqueuing the fallback. Type data as CreateDebtInput instead of redefining the request shape locally.
As per coding guidelines, UI request DTOs must use shared contracts and never be redefined locally.
🤖 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/debts/page.tsx` around lines 88 - 124, Update
handleCreate to accept the shared CreateDebtInput type and generate one client
request ID before the online attempt. Include that ID in profileApi.createDebt
and pass the same ID to enqueue when a NetworkError triggers queueLocally,
ensuring replay reuses the original request identity instead of creating a new
one.
Source: Coding guidelines
Comment on lines
+33
to
+39
| for (const entry of entries) { | ||
| try { | ||
| if (entry.kind === 'create-transaction') { | ||
| await profileApi.createTransaction(entry.userId, entry.payload); | ||
| } else { | ||
| await profileApi.createDebt(entry.userId, entry.payload); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Implement the required batch synchronization contract.
The PR requires an authenticated POST /api/users/:userId/sync endpoint with typed operations and per-operation results. The current client instead replays direct create requests, so that contract is neither invoked nor tested.
packages/ui/src/components/OfflineSync.tsx#L33-L39: submit queued operations to the batch sync endpoint and process each returned operation result.packages/ui/e2e/offline.spec.ts#L88-L94: assert the transaction operation is included in one sync request instead of asserting a direct transaction POST.packages/ui/e2e/offline.spec.ts#L123-L128: assert the debt operation is included in one sync request instead of asserting a direct debt POST.documentation/offline-support.md#L92-L100: update the design to describe the batch endpoint and its per-operation failure behavior.
📍 Affects 3 files
packages/ui/src/components/OfflineSync.tsx#L33-L39(this comment)packages/ui/e2e/offline.spec.ts#L88-L94packages/ui/e2e/offline.spec.ts#L123-L128documentation/offline-support.md#L92-L100
🤖 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/OfflineSync.tsx` around lines 33 - 39, The offline
synchronization flow must use the authenticated batch sync contract rather than
replaying direct create requests. In packages/ui/src/components/OfflineSync.tsx
lines 33-39, submit queued typed operations to POST /api/users/:userId/sync and
process each returned operation result, including per-operation failures. In
packages/ui/e2e/offline.spec.ts lines 88-94 and 123-128, update the transaction
and debt scenarios to assert each operation is included in one sync request
instead of asserting direct transaction or debt POSTs. In
documentation/offline-support.md lines 92-100, describe the batch endpoint and
its per-operation failure behavior.
Comment on lines
+42
to
+57
| } catch (err) { | ||
| // Still no network: stop and leave the rest queued for the next | ||
| // `online` event, keeping the replay in order. | ||
| if (err instanceof NetworkError) break; | ||
|
|
||
| // The server answered and rejected it. Retrying forever would never | ||
| // succeed and would block everything behind it, so drop the entry. | ||
| console.error('Dropping unsyncable offline entry', entry.kind, err); | ||
| remove(entry.id); | ||
| } | ||
| } | ||
| } finally { | ||
| isSyncing.current = false; | ||
| } | ||
|
|
||
| if (synced > 0) window.dispatchEvent(new CustomEvent(OFFLINE_SYNCED_EVENT)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the component structure first
ast-grep outline packages/ui/src/components/OfflineSync.tsx --view expanded || true
# Show the relevant section with line numbers
sed -n '1,220p' packages/ui/src/components/OfflineSync.tsx | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 3005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspecting OfflineSync.tsx...\n'
wc -l packages/ui/src/components/OfflineSync.tsx
sed -n '1,220p' packages/ui/src/components/OfflineSync.tsx | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 3002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the sync component, network error type, and event consumers.
rg -n "class NetworkError|NetworkError|OFFLINE_SYNCED_EVENT|Dropping unsyncable offline entry|offline synced|syncing" packages/ui/src -SRepository: ali-ahnaf/pocket_pixel
Length of output: 3213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the sync component and related definitions.
sed -n '1,220p' packages/ui/src/components/OfflineSync.tsx | cat -n
printf '\n--- NetworkError references ---\n'
rg -n "NetworkError|OFFLINE_SYNCED_EVENT" packages/ui/src -S
printf '\n--- files named around api/errors ---\n'
fd -a "NetworkError|error" packages/ui/src packages/ui/src/lib packages/ui/src/services 2>/dev/null || trueRepository: ali-ahnaf/pocket_pixel
Length of output: 5924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pull the smallest relevant slice and the error type definitions.
grep -n "catch (err)" -n packages/ui/src/components/OfflineSync.tsx
sed -n '35,70p' packages/ui/src/components/OfflineSync.tsx | cat -n
printf '\n--- definitions ---\n'
rg -n "class NetworkError|interface NetworkError|type NetworkError|new NetworkError|OFFLINE_SYNCED_EVENT" packages/ui/src -SRepository: ali-ahnaf/pocket_pixel
Length of output: 2917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- ApiClient error handling ---\n'
sed -n '1,180p' packages/ui/src/lib/api/ApiClient.ts | cat -n
printf '\n--- page sync consumer ---\n'
sed -n '180,255p' packages/ui/src/app/page.tsx | cat -n
printf '\n--- debts sync consumer ---\n'
sed -n '1,180p' packages/ui/src/app/debts/page.tsx | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- ApiClient response mapping ---\n'
sed -n '80,130p' packages/ui/src/lib/api/ApiClient.ts | cat -n
printf '\n--- page sync consumer ---\n'
sed -n '200,250p' packages/ui/src/app/page.tsx | cat -n
printf '\n--- debts sync consumer ---\n'
sed -n '70,140p' packages/ui/src/app/debts/page.tsx | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ApiClient response mapping ---'
sed -n '1,180p' packages/ui/src/lib/api/ApiClient.ts | cat -n
printf '%s\n' ''
printf '%s\n' '--- page sync consumer ---'
sed -n '190,250p' packages/ui/src/app/page.tsx | cat -n
printf '%s\n' ''
printf '%s\n' '--- debts sync consumer ---'
sed -n '70,140p' packages/ui/src/app/debts/page.tsx | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 12998
Dispatch OFFLINE_SYNCED_EVENT after terminal removals too. If every queued entry is rejected, synced stays 0 and the refetch event never fires, so the placeholder rows in page.tsx and debts/page.tsx can stay stale until something else triggers a reload.
🤖 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/OfflineSync.tsx` around lines 42 - 57, Update the
sync flow in OfflineSync so OFFLINE_SYNCED_EVENT is dispatched when entries are
successfully synchronized or terminally removed. Track whether any queue entry
was removed in the non-NetworkError path and use that alongside synced when
deciding to dispatch the event, while preserving the existing retry and ordering
behavior.
Comment on lines
+100
to
+109
| // No response at all means the request never reached the server. For the | ||
| // few allow-listed reference GETs, serve the last known payload so the | ||
| // offline-capable forms can still be filled in. A real 4xx/5xx keeps | ||
| // throwing, so genuine server failures are never masked by stale data. | ||
| if (!error.response) { | ||
| if (cacheableGet) { | ||
| const cached = readCache<T>(cacheKey); | ||
| if (cached !== null) return cached; | ||
| } | ||
| throw new NetworkError(error.message ?? 'Network request failed', error); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For Axios 1.16.0, can a timeout or connection reset after a server receives an HTTP request reject without error.response?
💡 Result:
In Axios 1.16.0, a timeout or connection reset generally results in an error where error.response is undefined [1][2]. Axios populates the error.response object only when the server has successfully returned an HTTP response (e.g., 4xx or 5xx status codes) [1][3]. When a request fails due to a timeout, network interruption, or connection reset before a response is received, Axios rejects the promise with an error (typically code ECONNABORTED for timeouts) that does not contain a response object [4][1][2]. However, there is a specific exception introduced and noted in version 1.16.0 regarding partial responses. Axios now preserves the partial response object on the AxiosError if a stream is aborted after the HTTP headers have already been received [5][6]. In this narrow scenario, error.response may exist even if the request was otherwise aborted or timed out, allowing access to headers or status information that arrived before the failure [5][6]. Except for this specific case of "headers-received" abortion, you should assume error.response is undefined for timeouts and connection resets, and your error-handling logic should check for the existence of error.response before attempting to access its properties [1][2].
Citations:
- 1: Axios, IOS 15.6 , undefined is not an object (evaluating 'error.response.status') axios/axios#5871
- 2: https://fixdevs.com/blog/axios-network-error/
- 3: https://deepwiki.com/axios/axios/2.6-error-handling
- 4: https://github.com/axios/axios/blob/e435384f/docs/pages/advanced/error-handling.md
- 5: https://npmx.dev/package-changelog/axios/v/1.16.0
- 6: https://newreleases.io/project/github/axios/axios/release/v1.16.0
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\b(clientRequestId|createTransaction|createDebt|enqueue)\b' \
packages/ui/src/lib/api/ProfileApi.ts \
packages/ui/src/lib/offline/outbox.ts \
packages/ui/src/components/LogResourceModal.tsx \
packages/ui/src/app/debts/page.tsxRepository: ali-ahnaf/pocket_pixel
Length of output: 7439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ProfileApi createTransaction / createDebt ==\n'
sed -n '160,250p' packages/ui/src/lib/api/ProfileApi.ts
printf '\n== outbox queue / replay ==\n'
sed -n '1,220p' packages/ui/src/lib/offline/outbox.ts
printf '\n== LogResourceModal transaction payload and catch branch ==\n'
sed -n '240,330p' packages/ui/src/components/LogResourceModal.tsx
printf '\n== shared transaction input DTOs ==\n'
rg -n -C2 'CreateTransactionInput|clientRequestId|CreateDebtInput' packages/shared packages/ui/src -g '!**/node_modules/**'Repository: ali-ahnaf/pocket_pixel
Length of output: 19832
Keep a stable clientRequestId on the first transaction attempt. enqueue() creates a new id only after NetworkError, so a request that reaches the server but loses its response can be retried with a different id and create a duplicate row. Attach and persist the id before the initial POST, then reuse it for replay.
🤖 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/api/ApiClient.ts` around lines 100 - 109, Update the
enqueue() transaction flow to generate and persist a stable clientRequestId
before the initial POST, including when the request reaches the server but loses
its response. Ensure the NetworkError retry/replay path reuses that persisted id
instead of creating a new one, preventing duplicate rows.
Comment on lines
+49
to
+50
| try { | ||
| window.localStorage.setItem(OUTBOX_STORAGE_KEY, JSON.stringify(entries)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid persisting financial payloads in plaintext.
Queued debt titles, amounts, notes, dates, and transaction details remain in origin-wide localStorage after the browser session. Use the existing session DEK or equivalent protected persistence before writing this expanded financial dataset.
🤖 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/offline/outbox.ts` around lines 49 - 50, Update the
persistence flow surrounding the localStorage write in the outbox module to
protect queued financial entries using the existing session DEK or equivalent
encrypted persistence before storage. Ensure debt titles, amounts, notes, dates,
and transaction details are never written as plaintext under OUTBOX_STORAGE_KEY,
while preserving the existing outbox queue behavior and error handling.
Comment on lines
+49
to
+76
| try { | ||
| window.localStorage.setItem(OUTBOX_STORAGE_KEY, JSON.stringify(entries)); | ||
| } catch { | ||
| // Out of quota: the write is lost, but throwing here would break the form | ||
| // the user just submitted. The queue stays consistent either way. | ||
| } | ||
| window.dispatchEvent(new CustomEvent(OUTBOX_CHANGED_EVENT)); | ||
| } | ||
|
|
||
| function newId(): string { | ||
| // randomUUID needs a secure context; fall back so a plain-HTTP dev host still works. | ||
| if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') return crypto.randomUUID(); | ||
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (char) => { | ||
| const random = (Math.random() * 16) | 0; | ||
| const value = char === 'x' ? random : (random & 0x3) | 0x8; | ||
| return value.toString(16); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Append an entry to the queue. The generated id doubles as the payload's | ||
| * `clientRequestId`, which is what makes the replay idempotent server-side. | ||
| */ | ||
| export function enqueue(entry: OutboxEntryInput): OutboxEntry { | ||
| const id = newId(); | ||
| const queued = { ...entry, id, queuedAt: new Date().toISOString(), payload: { ...entry.payload, clientRequestId: id } } as OutboxEntry; | ||
| write([...read(), queued]); | ||
| return queued; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not acknowledge an offline save when persistence failed.
write() swallows setItem failures, while enqueue() still returns an entry and callers close the form as saved. Quota or storage-policy failures therefore silently lose the write. Return/throw a storage failure from enqueue() and keep the form open with an actionable error; add coverage for this path.
🤖 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/offline/outbox.ts` around lines 49 - 76, The enqueue flow
must not report success when localStorage persistence fails. Update write and
enqueue so setItem errors propagate as a storage failure, allowing callers to
keep the form open and show an actionable error; preserve successful queueing
behavior and add coverage for the persistence-failure path.
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
New Features
Bug Fixes