Skip to content

fix: v3.5.12 — background.js cleanup (dead code, retry consistency, manifest declaration)#101

Merged
heznpc merged 1 commit intomainfrom
fix/v3.5.12-background-cleanup
May 10, 2026
Merged

fix: v3.5.12 — background.js cleanup (dead code, retry consistency, manifest declaration)#101
heznpc merged 1 commit intomainfrom
fix/v3.5.12-background-cleanup

Conversation

@heznpc
Copy link
Copy Markdown
Owner

@heznpc heznpc commented May 10, 2026

Summary

Deep audit of `src/background/background.js` (351 lines, the service worker — only partially reviewed in earlier rounds). Triaged 13 raw findings; 4 real bugs, the rest were over-broad or based on incorrect assumptions about MV3 SW lifecycle / JS concurrency.

# Severity What Fix
1 🔴 CRITICAL `_BG_YT_CLIENT_VERSION_DEFAULT` had storage-hydration self-healing but no code anywhere writes the storage key — purely dead. The "refreshed by maintenance alarm" comment was aspirational. Plain `const` + manual-bump workflow note. Eliminates the per-SW-wake storage race too.
2 🟠 HIGH `FETCH_URL` proxy used raw `fetch` — no 4xx-no-retry contract while GT path got it. Inconsistency the v3.5.8 fix was supposed to eliminate. Route through `fetchWithRetry`.
3 🟠 HIGH `handleVersionCheck` (GitHub API) also raw `fetch`. Anonymous GitHub quota is 60/h per IP; 403s common with hundreds of users on residential ranges. Route through `fetchWithRetry` (4xx fail-fast still avoids pointless retries on 403/404).
4 🟡 MEDIUM `api.github.com` not in `host_permissions` though SW fetches it. Works in MV3 but CWS reviewers flag undeclared origins. Add to manifest.

Also updated `scripts/check-bg-sync.js` to match the renamed constant.

Skipped findings (with reasons)

The agent surfaced more concerns; verified they don't hold:

  • Rate-limiter persistence across SW termination ("two tabs each spawn fresh SWs, doubling effective rate") — false. One SW per extension, shared across all tabs of the same browser profile.
  • Parallel `acquire()` polling races ("dozens may pass check() in same microtask") — false. JS single-thread; `check()` is synchronous and atomic on `this.timestamps`.
  • `sender.tab.url` validation for FETCH_URL — `host_permissions` already constrains `content_script` injection to `skilljar.com`; arbitrary frames can't trigger our handlers.
  • Origin/Referer header spoofing on InnerTube — required for the endpoint to work; not changeable.
  • Alarm idempotency hardening — works by accident, low value to add belt-and-suspenders.

Verification (local)

  • Tests: 336/336 pass across 13 suites
  • Lint / format clean
  • `check:selectors` / `check:dicts` / `check:sync` (updated for the renamed const) / `glossary` / `validate` — all green
  • `build:firefox` / `build:bundle` — both pass

Test plan

  • CI: validate + build + test green
  • Manual: trigger version-check (load extension, wait for the 7-day alarm or force-trigger via console) — confirm GitHub API call succeeds and "!" badge appears if a newer release exists
  • Manual: load a YouTube-embedded Skilljar lesson — confirm InnerTube subtitle path still works (no regression from FETCH_URL retry change)

🤖 Generated with Claude Code

…anifest)

Deep audit of the service worker found 4 real fixes after triaging
13 raw findings (the agent's rate-limiter persistence concerns
relied on incorrect assumptions about per-tab SW spawning and JS
concurrency — a single SW is shared across all tabs of an extension
and check() is atomic in single-threaded JS).

#1 Dead chrome.storage.local self-healing path removed
   `_BG_YT_CLIENT_VERSION_DEFAULT` was a `let` hydrated from
   chrome.storage.local on every SW wake, but a grep across src/
   shows nothing ever WRITES that key. The "refreshed by maintenance
   alarm" comment was aspirational — the alarm only sends version-
   check against GitHub. The runtime override never triggered.
   Replaced let + hydration block with a plain const + the actual
   manual-bump workflow note (synced via check-bg-sync.js).
   Also eliminates the fire-and-forget storage race that fired on
   every SW spin-up.

#2 FETCH_URL routes through fetchWithRetry
   Was using raw fetch with no 4xx-no-retry contract. A transient
   YouTube/InnerTube 5xx propagated straight to the content script
   while GOOGLE_TRANSLATE_BATCH had retries — the same inconsistency
   the v3.5.8 fix was supposed to eliminate.

#3 handleVersionCheck routes through fetchWithRetry
   Anonymous GitHub API quota is 60/h per IP; with users converging
   on residential ranges, 403s are common. Previously a single
   attempt silently dropped them. fetchWithRetry's 4xx fail-fast
   still avoids pointless retries on 403/404.

#4 api.github.com added to manifest host_permissions
   SW fetch to undeclared origins works in MV3 but CWS reviewers
   flag it. Explicit declaration matches what the code does.

Also: scripts/check-bg-sync.js updated to match the renamed
constant (was looking for _BG_YT_CLIENT_VERSION_DEFAULT).

Findings deliberately skipped, with reasons:
- Rate-limiter persistence across SW termination (agent: "two tabs
  spawn fresh SWs, doubling rate"). False — one SW per extension,
  shared across all tabs.
- Parallel acquire() polling races (agent: "many may pass check()
  in same microtask"). False — JS is single-threaded; check() is
  synchronous and atomic.
- Sender-tab.url validation. host_permissions already constrains
  content_script injection to skilljar.com.
- Origin/Referer header spoofing on InnerTube. Required for the
  endpoint to work at all; documented.

336/336 tests pass. Lint, format, selectors, dicts, sync, glossary,
validate, build:firefox, build:bundle all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@heznpc heznpc merged commit ae3eea5 into main May 10, 2026
3 checks passed
@heznpc heznpc deleted the fix/v3.5.12-background-cleanup branch May 10, 2026 15:35
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.

1 participant