-
Notifications
You must be signed in to change notification settings - Fork 0
Closed
Labels
Description
🐛 Bug Description
File: src/lib/analytics.ts (lines 113-122)
Severity: MEDIUM
Origin: Copilot review comment in PR #100
Problem
Race condition in debouncedSync() and syncEvents() coordination:
private debouncedSync(): void {
if (this.syncTimeout) {
clearTimeout(this.syncTimeout);
}
this.syncTimeout = window.setTimeout(() => {
this.syncEvents(); // ← Async sync triggered
this.syncTimeout = undefined;
}, 1000);
}
async syncEvents(): Promise<void> {
if (!this.isOnline || this.isDestroyed) return;
// ❌ No clearing of pending debounced sync!
if (this.isSyncing) {
console.log("Sync already in progress, skipping...");
return;
}
this.isSyncing = true;
// ... sync logic
}Issue: If syncEvents() is called directly (e.g., from handleOnline() at line 214) while a debounced sync is pending, the timeout is not cleared. This can cause:
- Duplicate sync attempts
- Race conditions between manual and debounced sync
- Inefficient network usage
Expected Behavior
When a manual sync is triggered, any pending debounced sync should be cancelled.
Proposed Solution
Clear pending timeout at the start of syncEvents():
async syncEvents(): Promise<void> {
if (!this.isOnline || this.isDestroyed) return;
// ✅ Clear any pending debounced sync
if (this.syncTimeout) {
clearTimeout(this.syncTimeout);
this.syncTimeout = undefined;
}
// Prevent concurrent syncs
if (this.isSyncing) {
console.log("Sync already in progress, skipping...");
return;
}
this.isSyncing = true;
// ... rest of sync logic
}Reproduction Steps
- Track multiple analytics events rapidly while online
- Debounced sync is scheduled for 1 second later
- Before timeout fires, manually trigger sync (e.g., by toggling offline/online)
- Both syncs may try to execute
Context
- Related to PR feat: Implement PWA Phase 3 features (Push Notifications, Share Target API, Offline Analytics) #100 (PWA Phase 3 implementation)
- Copilot comment: "Race condition in
syncEvents()debouncing" - Low impact in current implementation, but should be fixed for robustness
Acceptance Criteria
-
syncEvents()clears pendingsyncTimeoutat start - Manual sync cancels debounced sync
- No duplicate sync attempts detected
- Tests verify coordination between debounced and manual sync
Metadata
Metadata
Assignees
Labels
Type
Projects
Status
✅ Done