Skip to content

Fix memory leak, singleton session isolation, progress simulation, and settings validation#54

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/fix-memory-leak-upload-service
Draft

Fix memory leak, singleton session isolation, progress simulation, and settings validation#54
Copilot wants to merge 2 commits intomainfrom
copilot/fix-memory-leak-upload-service

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 18, 2026

Four reliability issues across upload lifecycle, session isolation, UX accuracy, and storage safety — plus a race condition in offscreen document creation.

Changes

UploadService.ts — callback memory leak + progress simulation

  • onUploadComplete() now returns an unsubscribe function; previously callbacks were inserted with a unique key but never removed, causing unbounded map growth and duplicate badge updates across restarts
  • Progress simulation replaced linear +0.1 steps with exponential decay (0.9 - current) * 0.1 per tick — bar no longer jumps to 90% and stalls for slow uploads
// before
onUploadComplete(callback): void { this.completionCallbacks.set(key, callback); }

// after
onUploadComplete(callback): () => void {
  this.completionCallbacks.set(key, callback);
  return () => this.completionCallbacks.delete(key);
}

NumbersApiManager.ts — singleton session isolation

  • Added resetNumbersApi() which nulls the module-level instance
  • clearAuth() calls it automatically — logout now guarantees the next session gets a clean singleton with no stale queue or callbacks from the previous user

StorageService.ts — storage boundary validation

  • Added private validateSettings() called by both setSettings() and updateSettings()
  • Guards: screenshotQuality must be a number in [0, 100]; huntModeMessage and huntModeHashtags capped at 280 characters (null-safe); invalid values throw before reaching storage

service-worker.ts — offscreen document race condition

  • Serializes concurrent ensureOffscreenDocument() calls via a module-level promise lock (offscreenDocumentPromise); concurrent callers (e.g. watermark + geolocation) now await the same in-flight promise instead of both racing past the context check and hitting createDocument twice
Original prompt

This section details on the original issue you should resolve

<issue_title>[Feature][Medium] Fix memory leaks, singleton reset, progress simulation, and settings validation</issue_title>
<issue_description>## Summary

Four medium-priority improvement findings across upload service reliability, singleton lifecycle, UX accuracy, and data validation.

Finding 1: Completion Callback Map Grows Unboundedly (Memory Leak)

File: src/services/UploadService.ts (lines 158-161)

onUploadComplete() registers callbacks in this.completionCallbacks using a unique key, but callbacks are never removed. Every call to getNumbersApi() adds a new entry. When a completion event fires, all registered callbacks execute (lines 187-193), causing duplicate badge updates or messages.

Note: Distinct from issue #44 which covers IndexedDB memory leaks. This is in the UploadService callback map.

Suggested Fix: Return an unsubscribe function from onUploadComplete(), or use a single callback slot.

Finding 2: NumbersApiManager Singleton Cannot Be Reset Between User Sessions

File: src/services/NumbersApiManager.ts (lines 150-159)

After clearAuth() during logout, the singleton retains all state. The UploadService may still have stale queue entries or callbacks from the previous user's session. There is no way to reset the singleton.

Impact: After logout and re-login on a shared device, completion callbacks from the old session persist and fire for the new session's uploads. Potential cross-user data leakage.

Note: Distinct from issue #29 which covers singleton leak in the context of init race. This is about data isolation between user sessions.

Suggested Fix:

export function resetNumbersApi(): void {
  instance = null;
}

Call this from the logout flow.

Finding 3: Progress Simulation Provides Misleading Upload Feedback

File: src/services/UploadService.ts (lines 284-297)

startProgressSimulation() artificially increments progress by 10% every 500ms to 90%, then stalls. For large screenshots on slow connections, the bar sits at 90% for the remaining upload. For fast uploads, it jumps from low percentages to 100%.

Suggested Fix: Use exponential approach: (0.9 - current) * 0.1 per tick, or show an indeterminate indicator. For precise progress, switch to XMLHttpRequest with upload.onprogress.

Finding 4: No Runtime Validation of Settings at Storage Boundary

File: src/services/StorageService.ts (lines 124-148)

setSettings() and updateSettings() store settings with no runtime type validation. A caller passing { screenshotQuality: "abc" } would store it and later cause runtime errors in chrome.tabs.captureVisibleTab(). huntModeMessage and huntModeHashtags accept arbitrary strings that flow into the share page's innerHTML.

Suggested Fix: Add runtime validation in setSettings():

private validateSettings(settings: Partial<StoredSettings>): void {
  if (settings.screenshotQuality !== undefined) {
    if (typeof settings.screenshotQuality !== 'number' || 
        settings.screenshotQuality < 0 || settings.screenshotQuality > 100) {
      throw new Error('Invalid screenshot quality');
    }
  }
  if (settings.huntModeMessage !== undefined && settings.huntModeMessage.length > 280) {
    throw new Error('Hunt mode message too long');
  }
}

Finding 5: Race Condition in Offscreen Document Creation

File: src/background/service-worker.ts (lines 386-399)

ensureOffscreenDocument() checks for existing contexts then creates one. Concurrent calls (e.g., watermark + geolocation simultaneously) can both pass the check and attempt creation, causing "Only a single offscreen document" error.

Suggested Fix: Use a promise lock to serialize calls:

let offscreenPromise: Promise<void> | null = null;
async function ensureOffscreenDocument() {
  if (offscreenPromise) return offscreenPromise;
  offscreenPromise = _ensureOffscreenDocument();
  try { await offscreenPromise; } finally { offscreenPromise = null; }
}
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…validation

Co-authored-by: numbers-official <181934381+numbers-official@users.noreply.github.com>
Copilot AI changed the title [WIP] [Feature] Fix memory leak in upload service callback map Fix memory leak, singleton session isolation, progress simulation, and settings validation Mar 18, 2026
Copilot AI requested a review from numbers-official March 18, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][Medium] Fix memory leaks, singleton reset, progress simulation, and settings validation

2 participants