Claude/offline sync audit 2ngdpr - #557
Conversation
Offline-sync audit, part 1 of 2. Each of these loses or corrupts crew work silently — nothing reaches the failed-sync surface, and in three of the five no delta pull will ever correct the divergence either. F1 — the optimistic local write and its outbox row were two separate IndexedDB transactions. A PWA reclaimed between them (iOS backgrounding, quota, a closed tab) left the cache updated with nothing queued to send it: the crew member sees their tick as saved forever, the server never hears about it, and because the server row's updated_at never changed, the delta pull never returns it. Adds enqueueMutationTx() and commits both writes in one Dexie transaction, in every helper plus photo-sync's applyUploadedPath. The processOutbox() kick stays outside the block — an IDB transaction auto-commits the moment an await leaves it. F2 — holdBackSuccessors() only saw successors that existed AT the moment of dead-lettering, which is the less likely half: the corrective edit is normally made after. Tick -> dead-letter -> un-tick (pushes fine) -> "Retry all" replays the stale tick on top and the server flips back. A record with a dead letter is now frozen at enqueue, so the whole sequence retries in order. F3 — logout with a second crew tab open. The shutdown latch is per-document module state but IndexedDB is per-origin, so the sibling tab held its connection, Dexie.delete blocked on it indefinitely, and the await before signOut()/redirect never resolved: logout silently did nothing and the cache stayed on a shared device. Adds a BroadcastChannel shutdown signal and bounds the delete so a tab that ignores it cannot strand the user mid-logout. F4 — discarding a dead letter removed the shadow overlay but not the cursor that had advanced past the server row it was masking, pinning the cache to a value the server never accepted. Same with no user action at all when the 30-day prune collects one. Adds invalidateCursorsFor() on both paths, plus forceFullCrewResync() as the repair path that did not exist (force was plumbed everywhere but never passed as true, and no cursor was ever reset). F5 — photo blobs live in a separate IndexedDB from their tracking rows, so the two can never be written atomically, and nothing collected a blob whose row never landed: megabytes each, until the browser evicts the whole origin and the mutation outbox with it. Adds a two-generation orphan sweep. A photo whose blob is gone now dead-letters instead of being deleted, which was indistinguishable from a successful upload. Also indexes `failed` (0/1 — IndexedDB cannot index a boolean, so every dead-letter query full-scanned the outbox, three of them live on every crew screen) and adds [table+targetId] for the per-record lookups F2 and holdBackSuccessors do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bs2uS5NYLR8Pvk4sw5NiKz
Offline-sync audit, part 2 of 2. Latency, invisible-state and forward- compatibility defects rather than outright data loss. F6 — a photo stuck in the transport-failure loop was invisible on every surface. A transport failure never sets `failed` (by design), so it fell out of the banner's dead-letter query, and the amber stalled notice only ever looked at db.mutations. A whole shift of verification photos could retry forever against a captive portal with nothing on screen, on a banner whose header comment claims it covers every queued photo. F7 — nothing serialized resync. A phone waking fires `online` and `visibilitychange` within the same second and the safety poll can join them, so up to three fullCrewResyncs ran concurrently on the worst possible connection: triple the queries, a race on advanceCursor's read-modify-write, and one pass's pruneLocalCache bulkDeleting from a snapshot another was mutating. Coalesced to one in flight plus one queued, the same shape createSyncSignalHandler already used per entity. F8 — a drain works from a snapshot, so a mutation queued mid-drain was invisible to it, and enqueueMutation's kick was dropped by the isProcessing guard. The row then waited for the next 30 s tick — during the reconnect window, where a crew member is most likely still working — and the bounded flush at logout could report "clean" for a row it never attempted. F10 — an outbox row can outlive the release that queued it. Payloads are now version-stamped with a migration hook, and a (table, op) with no handler is terminal rather than transient: it was a bare Error, so it burned five pointless round trips before dead-lettering with a developer string naming the table and op as its user-facing text. F11 — the per-record ordering invariant was enforced globally: any retryable failure or backoff window stopped the whole queue, so one flaky record stranded dozens of unrelated writes on reconnect. Now blocked per record — with a consecutive-distinct-failure circuit breaker, because per-record blocking alone would turn a server-side outage into N wasted requests per wave instead of one. F12 — checklist photo blob keys and storage paths were Date.now()-suffixed. Two captures in the same millisecond collided: the second overwrote the first blob, both rows referenced it, and the first row's cleanup deleted it out from under the second. The tracking row's own id already used crypto.randomUUID(). Lint ratchet 202 -> 201. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bs2uS5NYLR8Pvk4sw5NiKz
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
📝 WalkthroughWalkthroughThe PR hardens crew offline synchronization. It adds atomic local-write and outbox transactions, indexed numeric dead-letter flags, improved drain scheduling, cursor recovery, photo cleanup, cross-tab logout handling, and expanded failed-sync visibility and tests. ChangesOffline sync durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CrewAction
participant Dexie
participant SyncService
participant Supabase
CrewAction->>Dexie: write local state and enqueue mutation
CrewAction->>SyncService: start processing
SyncService->>Dexie: read eligible outbox rows
SyncService->>Supabase: upload mutation
Supabase-->>SyncService: success or terminal error
SyncService->>Dexie: complete or dead-letter mutation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
🧹 Nitpick comments (4)
unit/dexie/photo-sync-durability.test.ts (1)
289-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the untyped storage-client cast.
Line 290 bypasses the
SupabaseClientcontract. Define a narrow typed client interface forprocessPendingPhotoUploads()or provide a typed storage mock.As per coding guidelines, “Use concrete types or generics: no
any,as any, or@ts-ignore.”🤖 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 `@unit/dexie/photo-sync-durability.test.ts` around lines 289 - 290, Remove the `as any` cast and its eslint suppression in the `processPendingPhotoUploads` test call. Provide a typed storage mock or define a narrow client interface accepted by `processPendingPhotoUploads`, while preserving the existing test behavior and satisfying the `SupabaseClient` contract.Source: Coding guidelines
lib/dexie/photo-sync.ts (1)
116-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider deleting the queue row in the same transaction.
The transaction scope omits
pending_photo_uploads, so the caller removes the queue row in a separate transaction at Line 324. If the app is reclaimed between the two, the row stays pending and not failed. The next drain re-uploads the same object and callsapplyUploadedPath()again, which queues a second identical PATCH.The duplicate carries identical values and the upload uses
upsert: true, so no data is corrupted. It is wasted work on a metered connection. Addingdb.pending_photo_uploadsto the scope and deleting the row inside the block closes the window.🤖 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 `@lib/dexie/photo-sync.ts` around lines 116 - 117, Update the transaction in the photo-sync flow around applyUploadedPath() to include db.pending_photo_uploads, then delete the corresponding queue row within that same transaction instead of relying on the later separate deletion. Preserve the existing upload and mutation behavior while ensuring successful processing atomically removes the pending upload.lib/dexie/sync/cursors.ts (1)
117-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
ALL_CURSOR_KEYSfrom the map to prevent drift.
ALL_CURSOR_KEYSandCURSORS_BY_MUTATION_TABLElist the same four keys. If a future cursor is added to only the map,forceFullCrewResync()silently stops rewinding it, and the divergence-repair path stops repairing that table. The declaration also sits after the function that reads it; moving it above keeps the read order obvious.♻️ Proposed refactor
+const ALL_CURSOR_KEYS: readonly SyncCursorKey[] = [ + ...new Set(Object.values(CURSORS_BY_MUTATION_TABLE).flat()), +] as readonly SyncCursorKey[] + export async function resetAllCursors(userId: string): Promise<void> { const db = getDexieDb(userId) await Promise.all(ALL_CURSOR_KEYS.map((key) => db.sync_meta.delete(key))) } - -const ALL_CURSOR_KEYS: readonly SyncCursorKey[] = [ - 'cursor:turnovers', - 'cursor:checklist_instances', - 'cursor:checklist_items', - 'cursor:work_orders', -]If any cursor key must be reset without belonging to a mutation table, keep the explicit list and add a type-level exhaustiveness check instead.
🤖 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 `@lib/dexie/sync/cursors.ts` around lines 117 - 127, Derive ALL_CURSOR_KEYS from CURSORS_BY_MUTATION_TABLE instead of maintaining a duplicate four-key list, and move the derived declaration above resetAllCursors so its dependency is defined before use. Ensure the derived keys remain typed as SyncCursorKey and continue covering every cursor used by the resync and repair paths.CLAUDE.md (1)
458-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd guardrails for the three new mechanically-checkable conventions.
CLAUDE.mdstates that conventions added here require an ESLint rule orunit/guardrails/test. The first bullet namesunit/guardrails/crew-dead-letter-coverage.test.ts, but these do not:
- one Dexie transaction for the local write plus its outbox row
invalidateCursorsFor()on mutation discard pathsfailedwritten as0 | 1and never astrue/falseAdd the enforcement in the same PR, or remove/provide context for the mechanically checkable claims.
🤖 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 `@CLAUDE.md` around lines 458 - 479, Add ESLint rules or unit/guardrails tests for the three documented conventions: atomic local-write/outbox transactions around writeAndQueue()/enqueueMutationTx(), invalidateCursorsFor() on discardFailedMutation() and pruneExpiredDeadLetters() paths, and numeric 0/1 writes for the DeadLetterFlag failed field. Keep the CLAUDE.md claims only if these checks enforce them; otherwise remove or qualify the claims.Source: Learnings
🤖 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 `@lib/dexie/helpers.ts`:
- Around line 241-251: Update discardFailedMutation to wrap the mutation
deletion and invalidateCursorsFor call in one Dexie transaction covering
mutations and sync_meta, while preserving the existing mutation lookup and
conditional cursor rewind. Apply the same per-row transaction boundary to the
paired writes in pruneExpiredDeadLetters so deletion and cursor invalidation
cannot be separated by interruption.
In `@lib/dexie/photo-queue.ts`:
- Around line 105-114: Update listPendingPhotoBlobKeys to wrap the database read
and return flow in a try/finally, closing db in the finally block so both
success and getAllKeys failure release the connection. In the request error
handler, reject with a guaranteed Error value rather than nullable req.error,
preserving the existing key result behavior.
In `@lib/dexie/photo-sync.ts`:
- Around line 138-142: Guard row.storage_path at the start of applyUploadedPath
and fail fast when it is null, allowing the caller’s existing try/catch to
record the photo failure. Store the validated value in a local storagePath
variable, then use it for the property_assets update and all three
upload/mutation branches instead of row.storage_path or row.storage_path!;
preserve the existing success flow for non-null paths.
In `@lib/dexie/syncService.ts`:
- Around line 253-258: Update scheduleRetry() to preserve the earliest pending
retry time when multiple backing-off mutations are encountered, replacing the
existing timer only when the new wake-up is earlier. Also clear retryTimerAt in
dispose() alongside the retry timer so disposed services do not retain stale
scheduling state.
In `@unit/dexie/fake-dexie.ts`:
- Around line 18-21: Update the fakeCollection callbacks in the filter and
modify methods to use the concrete FakeRow parameter type instead of never/any
and remove the associated casts and lint suppression, while preserving the
existing matching and modification behavior.
- Around line 77-80: Update the anyOf implementation in fake-dexie.ts to match
compound index keys using matchesKey(row, fields, value) for each candidate
value, rather than comparing only fields[0]. Preserve the existing
fakeCollection filtering and primary-key handling while ensuring queries on
[table+targetId] exclude rows with nonmatching compound components.
In `@unit/dexie/outbox-drain-scheduling.test.ts`:
- Around line 91-101: Update the test setup around holder.gate and the wrapped
select() call so holder.gate.started resolves only when the first upload is
genuinely in flight. Await holder.gate.started after starting firstPass and
before seeding second or calling engine.processOutbox(), ensuring the test
exercises redrainRequested.
- Around line 129-132: Remove the `as any` cast from the `enqueueMutationTx`
call in the “stamps the current payload version” test. Update `FakeDexieDb` or
the `db()` helper to satisfy the typed Dexie handle contract, or route the test
through an appropriately typed adapter, while preserving the existing test
behavior.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 458-479: Add ESLint rules or unit/guardrails tests for the three
documented conventions: atomic local-write/outbox transactions around
writeAndQueue()/enqueueMutationTx(), invalidateCursorsFor() on
discardFailedMutation() and pruneExpiredDeadLetters() paths, and numeric 0/1
writes for the DeadLetterFlag failed field. Keep the CLAUDE.md claims only if
these checks enforce them; otherwise remove or qualify the claims.
In `@lib/dexie/photo-sync.ts`:
- Around line 116-117: Update the transaction in the photo-sync flow around
applyUploadedPath() to include db.pending_photo_uploads, then delete the
corresponding queue row within that same transaction instead of relying on the
later separate deletion. Preserve the existing upload and mutation behavior
while ensuring successful processing atomically removes the pending upload.
In `@lib/dexie/sync/cursors.ts`:
- Around line 117-127: Derive ALL_CURSOR_KEYS from CURSORS_BY_MUTATION_TABLE
instead of maintaining a duplicate four-key list, and move the derived
declaration above resetAllCursors so its dependency is defined before use.
Ensure the derived keys remain typed as SyncCursorKey and continue covering
every cursor used by the resync and repair paths.
In `@unit/dexie/photo-sync-durability.test.ts`:
- Around line 289-290: Remove the `as any` cast and its eslint suppression in
the `processPendingPhotoUploads` test call. Provide a typed storage mock or
define a narrow client interface accepted by `processPendingPhotoUploads`, while
preserving the existing test behavior and satisfying the `SupabaseClient`
contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c82b8f4-8fe5-447e-89d3-0f1a0162caf1
📒 Files selected for processing (26)
CLAUDE.mdapp/crew/_components/failed-sync-banner.tsxapp/crew/crew-shell.tsxapp/crew/turnovers/[id]/use-turnover-actions.tslib/dexie/context.tsxlib/dexie/demo-readiness.tslib/dexie/helpers.tslib/dexie/net.tslib/dexie/photo-queue.tslib/dexie/photo-sync.tslib/dexie/prune.tslib/dexie/schema.tslib/dexie/sync/cursors.tslib/dexie/sync/full-resync.tslib/dexie/syncService.tspackage.jsonunit/demo/demo-readiness.test.tsunit/dexie/fake-dexie.tsunit/dexie/offline-write-durability.test.tsunit/dexie/outbox-drain-scheduling.test.tsunit/dexie/photo-sync-durability.test.tsunit/dexie/sync-outbox-backoff.test.tsunit/dexie/sync-outbox-durability.test.tsunit/dexie/sync-outbox-ordering.test.tsunit/dexie/sync-shadow-and-prune.test.tsunit/guardrails/crew-dead-letter-coverage.test.ts
| export async function discardFailedMutation(userId: string, mutationId: number): Promise<void> { | ||
| const db = getDexieDb(userId) | ||
| const mutation = await db.mutations.get(mutationId) | ||
| await db.mutations.delete(mutationId) | ||
|
|
||
| // Abandoning the write hands authority back to the server — but the pull | ||
| // that would fetch the server's value has already moved its cursor past | ||
| // that row (see invalidateCursorsFor). Without this rewind the local cache | ||
| // stays pinned to a value the server never accepted, forever and silently. | ||
| if (mutation) await invalidateCursorsFor(userId, mutation.table) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Commit the delete and the cursor rewind in one transaction.
discardFailedMutation() deletes the mutation row, then rewinds the cursor in a separate IndexedDB transaction. If the app is reclaimed between the two writes, the row is gone but the cursor still sits past the server row it masked. The delta filter then skips that row forever, which is the permanent silent divergence this PR documents as unacceptable. The window is the same one writeAndQueue() was added to close.
Both writes touch only mutations and sync_meta, so one transaction is enough. invalidateCursorsFor() calls getDexieDb(userId) internally and joins the enclosing Dexie transaction, so keep sync_meta in scope.
🛠️ Proposed fix
export async function discardFailedMutation(userId: string, mutationId: number): Promise<void> {
const db = getDexieDb(userId)
- const mutation = await db.mutations.get(mutationId)
- await db.mutations.delete(mutationId)
-
- // Abandoning the write hands authority back to the server — but the pull
- // that would fetch the server's value has already moved its cursor past
- // that row (see invalidateCursorsFor). Without this rewind the local cache
- // stays pinned to a value the server never accepted, forever and silently.
- if (mutation) await invalidateCursorsFor(userId, mutation.table)
+ // The delete and the rewind must commit together. As two transactions, a
+ // process killed between them leaves the row gone with the cursor still
+ // advanced past the server row it masked — the delta filter then skips that
+ // row forever (see invalidateCursorsFor).
+ await db.transaction('rw', [db.mutations, db.sync_meta], async () => {
+ const mutation = await db.mutations.get(mutationId)
+ await db.mutations.delete(mutationId)
+ if (mutation) await invalidateCursorsFor(userId, mutation.table)
+ })
}pruneExpiredDeadLetters() in lib/dexie/prune.ts pairs the same two writes per row and needs the same treatment.
Based on learnings: "Protect load-decide-write sequences against races with database constraints or atomic updates, and provide cleanup or rollback paths for partial multi-step writes."
📝 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.
| export async function discardFailedMutation(userId: string, mutationId: number): Promise<void> { | |
| const db = getDexieDb(userId) | |
| const mutation = await db.mutations.get(mutationId) | |
| await db.mutations.delete(mutationId) | |
| // Abandoning the write hands authority back to the server — but the pull | |
| // that would fetch the server's value has already moved its cursor past | |
| // that row (see invalidateCursorsFor). Without this rewind the local cache | |
| // stays pinned to a value the server never accepted, forever and silently. | |
| if (mutation) await invalidateCursorsFor(userId, mutation.table) | |
| } | |
| export async function discardFailedMutation(userId: string, mutationId: number): Promise<void> { | |
| const db = getDexieDb(userId) | |
| // The delete and the rewind must commit together. As two transactions, a | |
| // process killed between them leaves the row gone with the cursor still | |
| // advanced past the server row it masked — the delta filter then skips that | |
| // row forever (see invalidateCursorsFor). | |
| await db.transaction('rw', [db.mutations, db.sync_meta], async () => { | |
| const mutation = await db.mutations.get(mutationId) | |
| await db.mutations.delete(mutationId) | |
| if (mutation) await invalidateCursorsFor(userId, mutation.table) | |
| }) | |
| } |
🤖 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 `@lib/dexie/helpers.ts` around lines 241 - 251, Update discardFailedMutation to
wrap the mutation deletion and invalidateCursorsFor call in one Dexie
transaction covering mutations and sync_meta, while preserving the existing
mutation lookup and conditional cursor rewind. Apply the same per-row
transaction boundary to the paired writes in pruneExpiredDeadLetters so deletion
and cursor invalidation cannot be separated by interruption.
Source: Learnings
| export async function listPendingPhotoBlobKeys(userId: string): Promise<string[]> { | ||
| const db = await openDb(userId) | ||
| const keys = await new Promise<string[]>((resolve, reject) => { | ||
| const req = db.transaction(STORE_NAME, 'readonly').objectStore(STORE_NAME).getAllKeys() | ||
| req.onsuccess = () => resolve(req.result as string[]) | ||
| req.onerror = () => reject(req.error) | ||
| }) | ||
| db.close() | ||
| return keys | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the database when the key read fails.
db.close() runs only on the success path. If getAllKeys() rejects, the IDBDatabase connection stays open for the lifetime of the document. pruneOrphanPhotoBlobs() in lib/dexie/prune.ts swallows that rejection, so nothing else closes it. An open connection also blocks indexedDB.deleteDatabase, which is the exact condition deleteDbBounded() in lib/dexie/schema.ts now abandons after 3 s — so a failed prune can make logout leave the photo store behind.
Use try/finally. The same fix also resolves the SonarCloud note at Line 110: req.error is DOMException | null, so a null error rejects with a non-Error value.
🛠️ Proposed fix
export async function listPendingPhotoBlobKeys(userId: string): Promise<string[]> {
const db = await openDb(userId)
- const keys = await new Promise<string[]>((resolve, reject) => {
- const req = db.transaction(STORE_NAME, 'readonly').objectStore(STORE_NAME).getAllKeys()
- req.onsuccess = () => resolve(req.result as string[])
- req.onerror = () => reject(req.error)
- })
- db.close()
- return keys
+ try {
+ return await new Promise<string[]>((resolve, reject) => {
+ const req = db.transaction(STORE_NAME, 'readonly').objectStore(STORE_NAME).getAllKeys()
+ req.onsuccess = () => resolve(req.result as string[])
+ req.onerror = () => reject(req.error ?? new Error('photo blob key read failed'))
+ })
+ } finally {
+ db.close()
+ }
}📝 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.
| export async function listPendingPhotoBlobKeys(userId: string): Promise<string[]> { | |
| const db = await openDb(userId) | |
| const keys = await new Promise<string[]>((resolve, reject) => { | |
| const req = db.transaction(STORE_NAME, 'readonly').objectStore(STORE_NAME).getAllKeys() | |
| req.onsuccess = () => resolve(req.result as string[]) | |
| req.onerror = () => reject(req.error) | |
| }) | |
| db.close() | |
| return keys | |
| } | |
| export async function listPendingPhotoBlobKeys(userId: string): Promise<string[]> { | |
| const db = await openDb(userId) | |
| try { | |
| return await new Promise<string[]>((resolve, reject) => { | |
| const req = db.transaction(STORE_NAME, 'readonly').objectStore(STORE_NAME).getAllKeys() | |
| req.onsuccess = () => resolve(req.result as string[]) | |
| req.onerror = () => reject(req.error ?? new Error('photo blob key read failed')) | |
| }) | |
| } finally { | |
| db.close() | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 110-110: Expected the Promise rejection reason to be an Error.
🤖 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 `@lib/dexie/photo-queue.ts` around lines 105 - 114, Update
listPendingPhotoBlobKeys to wrap the database read and return flow in a
try/finally, closing db in the finally block so both success and getAllKeys
failure release the connection. In the request error handler, reject with a
guaranteed Error value rather than nullable req.error, preserving the existing
key result behavior.
Source: Linters/SAST tools
| await db.property_assets.update(row.target_id, { photo_url: row.storage_path! }) | ||
| await enqueueMutationTx(db, 'property_assets', row.target_id, 'PATCH', { | ||
| photo_url: row.storage_path, | ||
| scanRequest: { storagePath: row.storage_path, mediaType: 'image/jpeg' }, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not assert storage_path non-null; guard it.
PendingPhotoUploadRow.storage_path is typed string | null. Line 138 uses ! to silence that. If the field is null, the local row stores null and the queued payload carries photo_url: null, so the drain writes a real NULL over the server value. This is the same failure shape as the ?? null rule for upload builders: a write that succeeds while erasing data.
The two branches above have the same exposure — Line 129 forwards row.storage_path directly.
Fail fast instead. The caller wraps applyUploadedPath() in the try block at Line 300, so a throw routes to recordPhotoFailure() and lands on the failed-sync surface.
🛠️ Proposed fix
async function applyUploadedPath(
userId: string,
row: PendingPhotoUploadRow,
): Promise<void> {
const db = getDexieDb(userId)
+ // A row with no storage_path has nothing to write. Assigning it anyway
+ // writes a real NULL over the server's value.
+ const storagePath = row.storage_path
+ if (!storagePath) {
+ throw new UploadDataError(`photo ${row.id}: no storage path to record`, 'NO_FIELDS')
+ }Then use storagePath in place of row.storage_path and row.storage_path! in all three branches.
As per coding guidelines: "Dexie upload builders may assign a field from a mutation payload only after checking that the field is present; never use payload.field ?? null for omitted fields." and "handle nullable database fields explicitly".
🤖 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 `@lib/dexie/photo-sync.ts` around lines 138 - 142, Guard row.storage_path at
the start of applyUploadedPath and fail fast when it is null, allowing the
caller’s existing try/catch to record the photo failure. Store the validated
value in a local storagePath variable, then use it for the property_assets
update and all three upload/mutation branches instead of row.storage_path or
row.storage_path!; preserve the existing success flow for non-null paths.
Source: Coding guidelines
| /** True when this mutation is still inside its retry window (and schedules the resume). */ | ||
| private isBackingOff(mutation: MutationRow): boolean { | ||
| if (mutation.nextAttemptAt === undefined || mutation.nextAttemptAt <= Date.now()) return false | ||
| this.scheduleRetry(mutation.nextAttemptAt) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the earliest wake-up, not the last one scheduled.
scheduleRetry() holds one timer and clears the previous one on every call. Before this change the drain stopped at the first not-due mutation, so exactly one wake-up was scheduled per pass and the last writer was also the only writer.
With per-record blocking the loop now continues past a backing-off record, so isBackingOff() can call scheduleRetry() several times in one pass. The last record seen wins. If record A is due in 2 s and record D is due in 5 min, the timer is set for 5 min and A waits for D. The existing "an overwritten later wake-up self-corrects on the next drain" note at Line 88 no longer holds, because there may be no other drain trigger while the crew member is offline.
Make scheduleRetry() keep the earliest pending wake-up.
🛠️ Proposed fix
+ // Earliest pending wake-up, so a later record's long backoff window cannot
+ // push out an earlier record's short one.
+ private retryTimerAt: number | null = null
+
private scheduleRetry(nextAttemptAt: number): void {
- if (this.retryTimer !== null) clearTimeout(this.retryTimer)
if (this.disposed) return
+ if (this.retryTimer !== null) {
+ if (this.retryTimerAt !== null && this.retryTimerAt <= nextAttemptAt) return
+ clearTimeout(this.retryTimer)
+ }
+ this.retryTimerAt = nextAttemptAt
this.retryTimer = setTimeout(() => {
this.retryTimer = null
+ this.retryTimerAt = null
if (this.disposed) return
void this.processOutbox()
}, Math.max(0, nextAttemptAt - Date.now()))
}Clear retryTimerAt in dispose() as well.
🤖 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 `@lib/dexie/syncService.ts` around lines 253 - 258, Update scheduleRetry() to
preserve the earliest pending retry time when multiple backing-off mutations are
encountered, replacing the existing timer only when the new wake-up is earlier.
Also clear retryTimerAt in dispose() alongside the retry timer so disposed
services do not retain stale scheduling state.
| filter: (predicate: (row: never) => boolean) => | ||
| fakeCollection(matches.filter((r) => predicate(r as never)), pk), | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| modify: async (apply: (row: any) => void) => { for (const r of matches) apply(r) }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate fake-dexie.ts =="
fd -a 'fake-dexie\.ts$' . || true
echo "== git status/stat =="
git status --short || true
echo "== file excerpt =="
for f in $(fd 'fake-dexie\.ts$' .); do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done
echo "== references =="
rg -n "fakeCollection|class.*Fake|FakeRow|matches\.filter|modify|filter:" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: smj1860/fieldstay
Length of output: 16127
Type fakeCollection callbacks with FakeRow.
filter and modify use never/any callbacks in unit/dexie/fake-dexie.ts. Replace these casts with a concrete row parameter type, such as FakeRow, so fake-table callbacks are not hidden.
🤖 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 `@unit/dexie/fake-dexie.ts` around lines 18 - 21, Update the fakeCollection
callbacks in the filter and modify methods to use the concrete FakeRow parameter
type instead of never/any and remove the associated casts and lint suppression,
while preserving the existing matching and modification behavior.
Source: Coding guidelines
| anyOf: (values: unknown[]) => { | ||
| const wanted = new Set(values) | ||
| const matches = [...rows.values()].filter((r) => wanted.has(r[field])) | ||
| return { | ||
| primaryKeys: async () => matches.map((r) => r[pk]), | ||
| toArray: async () => matches, | ||
| } | ||
| return fakeCollection([...rows.values()].filter((r) => wanted.has(r[fields[0]!])), pk) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'fake-dexie\.ts$' . || true
file="$(fd 'fake-dexie\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Outline: $file"
ast-grep outline "$file" || true
echo
echo "Relevant contents:"
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo
echo "Search matchesKey/anyOf usages:"
rg -n "matchesKey|anyOf\(" "$(dirname "$file")" . || trueRepository: smj1860/fieldstay
Length of output: 9619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(fd 'fake-dexie\.ts$' . | head -n 1 | xargs dirname || true)"
echo "root: $root"
echo
echo "Candidate dexie fake files:"
fd -i 'fake.*|dexie.*' "$root" . || true
echo
echo "Exact references to fakeDexie/fakeTable/fakeDexieDb:"
rg -n "fakeDexie|fakeTable|makeFakeDexieDb|turnovers|checklist|property_assets|anyOf\\(" . || trueRepository: smj1860/fieldstay
Length of output: 50374
Match compound keys in anyOf().
anyOf() only compares indexFields(index)[0], so a query on [table+targetId] can return rows with the wrong targetId. Use matchesKey(row, fields, value) for each candidate value here too.
Proposed fix
- anyOf: (values: unknown[]) => {
- const wanted = new Set(values)
- return fakeCollection([...rows.values()].filter((r) => wanted.has(r[fields[0]!])), pk)
- },
+ anyOf: (values: unknown[]) =>
+ fakeCollection(
+ [...rows.values()].filter((row) =>
+ values.some((value) => matchesKey(row, fields, value)),
+ ),
+ pk,
+ ),📝 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.
| anyOf: (values: unknown[]) => { | |
| const wanted = new Set(values) | |
| const matches = [...rows.values()].filter((r) => wanted.has(r[field])) | |
| return { | |
| primaryKeys: async () => matches.map((r) => r[pk]), | |
| toArray: async () => matches, | |
| } | |
| return fakeCollection([...rows.values()].filter((r) => wanted.has(r[fields[0]!])), pk) | |
| }, | |
| anyOf: (values: unknown[]) => | |
| fakeCollection( | |
| [...rows.values()].filter((row) => | |
| values.some((value) => matchesKey(row, fields, value)), | |
| ), | |
| pk, | |
| ), |
🤖 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 `@unit/dexie/fake-dexie.ts` around lines 77 - 80, Update the anyOf
implementation in fake-dexie.ts to match compound index keys using
matchesKey(row, fields, value) for each candidate value, rather than comparing
only fields[0]. Preserve the existing fakeCollection filtering and primary-key
handling while ensuring queries on [table+targetId] exclude rows with
nonmatching compound components.
| let release!: () => void | ||
| const promise = new Promise<void>((resolve) => { release = resolve }) | ||
| holder.gate = { promise, release, started: Promise.resolve() } | ||
|
|
||
| const engine = new SyncEngine('u1') | ||
| const firstPass = engine.processOutbox() | ||
|
|
||
| // Land a new mutation while the first push is genuinely in flight, and | ||
| // ring the bell the same way enqueueMutation() does. | ||
| await seed({ targetId: 'second' }) | ||
| await engine.processOutbox() // swallowed by isProcessing — must be REMEMBERED |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wait until the first upload is in flight.
holder.gate.started is already resolved and this test never awaits it. The second mutation can be added before the first drain takes its snapshot. The test can then pass without exercising redrainRequested.
Resolve a deferred started promise from the wrapped select() call. Await that promise before adding second.
🤖 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 `@unit/dexie/outbox-drain-scheduling.test.ts` around lines 91 - 101, Update the
test setup around holder.gate and the wrapped select() call so
holder.gate.started resolves only when the first upload is genuinely in flight.
Await holder.gate.started after starting firstPass and before seeding second or
calling engine.processOutbox(), ensuring the test exercises redrainRequested.
| it('stamps the current payload version on every newly queued mutation', async () => { | ||
| const { enqueueMutationTx } = await import('@/lib/dexie/syncService') | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- fake db, not a real FieldStayDexie | ||
| await enqueueMutationTx(db() as any, 'inventory_items', 'item1', 'PATCH', { current_quantity: 1 }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching outbox-drain-scheduling.test.ts:\n'
fd -a 'outbox-drain-scheduling\.test\.ts$' . || true
file="$(fd 'outbox-drain-scheduling\.test\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nRelevant lines 100-150:\n'
sed -n '100,150p' "$file" | nl -ba -v100
fi
printf '\nDefinitions/usages of enqueueMutationTx:\n'
rg -n "enqueueMutationTx|FakeDexieDb|FieldStayDexie|type .*Dexie|interface .*Dexie" -S .Repository: smj1860/fieldstay
Length of output: 432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant lines 100-150:\n'
sed -n '100,150p' ./unit/dexie/outbox-drain-scheduling.test.ts
printf '\nFile: ./unit/dexie/outbox-drain-scheduling.test.ts\n'
wc -l ./unit/dexie/outbox-drain-scheduling.test.ts
printf '\nDefinitions/usages of enqueueMutationTx:\n'
rg -n "enqueueMutationTx|FakeDexieDb|FieldStayDexie|type .*Dexie|interface .*Dexie|database|Dexie" -S . --glob '*.ts' --glob '*.tsx'Repository: smj1860/fieldstay
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Fake Dexie implementation:\n'
sed -n '1,140p' ./unit/dexie/fake-dexie.ts
printf '\nScheduler test imports/setup and related calls:\n'
sed -n '1,70p' ./unit/dexie/outbox-drain-scheduling.test.ts
printf '\nenqueueMutationTx implementation around signature/body:\n'
sed -n '860,905p' ./lib/dexie/syncService.ts
printf '\nMutationRow and FieldStayDexie declarations:\n'
sed -n '1,140p' ./lib/dexie/schema.ts
sed -n '230,315p' ./lib/dexie/schema.ts
sed -n '630,695p' ./lib/dexie/schema.ts
printf '\nRead-only signature/body summary from files:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('./lib/dexie/syncService.ts')
text = p.read_text()
start = text.index('export async function enqueueMutationTx(')
end = text.find('\nexport async function', start+1)
print(text[start:end if end != -1 else None])
PYRepository: smj1860/fieldstay
Length of output: 25422
Remove the as any database cast.
enqueueMutationTx() requires a typed Dexie handle, but this test passes FakeDexieDb through db() as any. Make FakeDexieDb satisfy the contract, or put this test on a typed adapter/helper instead.
🤖 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 `@unit/dexie/outbox-drain-scheduling.test.ts` around lines 129 - 132, Remove
the `as any` cast from the `enqueueMutationTx` call in the “stamps the current
payload version” test. Update `FakeDexieDb` or the `db()` helper to satisfy the
typed Dexie handle contract, or route the test through an appropriately typed
adapter, while preserving the existing test behavior.
Source: Coding guidelines
2 participants
Summary by CodeRabbit
New Features
Bug Fixes