Skip to content

Refactor duplicated capture logic, fix format bug, race condition, and dead code#16

Draft
Copilot wants to merge 3 commits intomainfrom
copilot/refactor-capture-logic
Draft

Refactor duplicated capture logic, fix format bug, race condition, and dead code#16
Copilot wants to merge 3 commits intomainfrom
copilot/refactor-capture-logic

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 11, 2026

Five code quality and correctness issues: ~350 lines of duplicated capture logic, a dead service file, watermark always outputting PNG regardless of user format setting, a singleton race condition, and createdAt type mismatches.

Changes

Extract shared capture helpers (service-worker.ts)

Pulled duplicated logic from handleSelectionComplete and handleScreenshotCapture into three shared helpers:

  • getCaptureDependencies(tab, settings) — geolocation + source website metadata
  • createAndStoreAsset(...) — asset construction and IndexedDB write
  • handlePostCapture(asset, settings, fromPopup) — notification, badge, auto-upload queue

Delete ScreenshotService.ts

278-line file from an earlier architecture, never imported anywhere. Removed entirely.

Fix watermark format (offscreen.ts)

canvas.toDataURL() was hardcoded to 'image/png'. Added format?: 'png' | 'jpeg' to WatermarkPayload and both ADD_WATERMARK call sites now pass settings.screenshotFormat, so JPEG captures are no longer silently re-encoded as PNG.

// Before
return { dataUrl: canvas.toDataURL('image/png') };

// After
const mimeType = payload.format === 'jpeg' ? 'image/jpeg' : 'image/png';
return { dataUrl: canvas.toDataURL(mimeType) };

Fix getNumbersApi() race condition (NumbersApiManager.ts)

Concurrent calls before initialization completed could trigger duplicate initialize() calls. Replaced the instance variable with a cached promise:

let initPromise: Promise<NumbersApiManager> | null = null;

export function getNumbersApi(): Promise<NumbersApiManager> {
  if (!initPromise) {
    initPromise = (async () => {
      const inst = new NumbersApiManager();
      await inst.initialize();
      return inst;
    })();
  }
  return initPromise;
}

Fix createdAt type mismatch (popup.tsx)

Two createdAt assignments used new Date().toISOString() (string) behind as any casts. Replaced with Date.now() to match Asset.createdAt: number.

Original prompt

This section details on the original issue you should resolve

<issue_title>[Feature][High] Refactor duplicated capture logic and remove dead code</issue_title>
<issue_description>## Summary

Significant code duplication and dead code that increases maintenance burden and bug risk.

Finding 1: ~350 Lines of Duplicated Screenshot Capture Logic

File: src/background/service-worker.ts

handleScreenshotCapture() (lines 404-581) and handleSelectionComplete() (lines 205-380) contain near-identical logic for:

  • Getting settings from storage
  • Capturing geolocation
  • Capturing website metadata
  • Creating asset objects
  • Storing assets in IndexedDB
  • Auto-upload queueing
  • Sending notifications

Any bug fix or feature change must be applied to both locations, creating a high risk of divergence.

Fix: Extract shared logic into helper functions:

async function getCaptureDependencies() { /* settings, geo, metadata */ }
async function createAndStoreAsset(dataUrl, dependencies) { /* asset creation, storage */ }
async function handlePostCapture(asset, settings) { /* auto-upload, notifications */ }

Finding 2: Dead Code — ScreenshotService.ts (278 lines)

File: src/services/ScreenshotService.ts (entire file)

This service is never imported anywhere in the codebase. The service worker handles capture directly via chrome.tabs.captureVisibleTab() and delegates watermarking to the offscreen document. This file is from an earlier architecture and should be removed.

Fix: Delete src/services/ScreenshotService.ts entirely.

Finding 3: Watermark Always Outputs PNG Regardless of User Setting

File: src/offscreen/offscreen.ts, line 166

return { dataUrl: canvas.toDataURL('image/png') };

Even when users select JPEG format in settings, watermarked screenshots are always output as PNG. This can significantly increase file size (PNG vs JPEG for photographic content).

Fix: Pass the user's format preference to the offscreen document and use it in canvas.toDataURL().

Finding 4: Race Condition in Singleton Initialization

File: src/services/NumbersApiManager.ts, lines 151-159

Concurrent calls to getNumbersApi() before initialization completes can trigger duplicate initialize() calls.

Fix: Use a promise-based singleton pattern:

let initPromise: Promise<NumbersApiManager> | null = null;
export function getNumbersApi(): Promise<NumbersApiManager> {
  if (!initPromise) {
    initPromise = (async () => {
      const inst = new NumbersApiManager();
      await inst.initialize();
      return inst;
    })();
  }
  return initPromise;
}

Finding 5: Incorrect Type Assertion on createdAt

File: src/popup/popup.tsx, lines 89, 258

Asset.createdAt is typed as number but ISO date strings are assigned with as any casts, masking type errors.

Fix: Use Date.now() instead of new Date().toISOString() for createdAt.

Impact

  • Duplicated code is the numbersprotocol/proofsnap-extension#1 source of inconsistent behavior and missed bug fixes
  • Dead code confuses contributors and inflates bundle size
  • The PNG-only output bug directly affects users who chose JPEG format

Suggested Approach

  1. Extract shared capture helpers from service-worker.ts (~350 lines saved)
  2. Delete ScreenshotService.ts
  3. Fix offscreen document to respect format setting
  4. Fix singleton race condition
  5. Fix type mismatches (remove as any casts)

Generated by Health Monitor with Omni</issue_description>

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


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits March 11, 2026 13:52
Co-authored-by: numbers-official <181934381+numbers-official@users.noreply.github.com>
- Extract getCaptureDependencies, createAndStoreAsset, handlePostCapture helpers
  from duplicated code in handleSelectionComplete and handleScreenshotCapture
- Delete dead code src/services/ScreenshotService.ts (never imported anywhere)
- Fix offscreen.ts to use user format preference in canvas.toDataURL()
- Fix race condition in NumbersApiManager.getNumbersApi() with promise-based singleton
- Fix type mismatch: use Date.now() instead of new Date().toISOString() for createdAt

Co-authored-by: numbers-official <181934381+numbers-official@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor duplicated capture logic and remove dead code Refactor duplicated capture logic, fix format bug, race condition, and dead code Mar 11, 2026
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][High] Refactor duplicated capture logic and remove dead code

2 participants