feat(extension): durable queue, modular ESM, and CI hardening - #10
Merged
Merged
Conversation
src/lib/url.js — first ESM module under src/lib/. Pure functions: - normalizeServerUrl: trim, lowercase scheme/host, strip trailing slashes and a trailing /api suffix. Returns '' on unparseable input. - isValidServerUrl: thin boolean predicate over normalizeServerUrl. 14 tests cover happy paths, edge cases (whitespace, null/undefined, non-string), and rejection (ftp, malformed URLs, missing host). 100% line/function coverage; coverage threshold now ratchets active. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
createStorage(area) returns { get, set, remove, clear } over
chrome.storage.local or .session. Default-value support distinguishes
'absent' from 'present and undefined' via hasOwnProperty.
8 tests cover: roundtrip, default values, multi-key remove, clear,
local/session isolation, and unknown-area rejection.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
src/lib/config.js — load/save/isConfigured over chrome.storage.local under a versioned key (whatsorga_config_v1). Validates serverUrl via the url module, normalises whitespace on apiKey, dedupes whitelist case-insensitively while preserving the first-seen casing. 8 tests cover defaults, roundtrip, invalid URL rejection, partial patch merge, whitelist edge cases, and boolean coercion. The two @ts-expect-error markers exist on tests that deliberately exercise invalid input — JSDoc strict mode catches those at the call site, which is the behaviour we want for production callers. 100% line/function coverage on config.js + storage.js + url.js. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Delete the legacy MessageQueue/QueueManager module — all durability now lives in src/lib/queue.js (chrome.storage.session) + src/lib/router.js. Manifest updated to remove the stale script entry. Fix transport.js catch clause typing (@type {any} cast). Add @ts-nocheck to background.js and content.js deferring DOM null-checks to Task 3.6. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix all TypeScript errors in the three entry-point files: - background.js: guard tab.id against undefined before sendMessage - content.js: getAttribute ?? fallback, HTMLAudioElement cast, any-cast catch clause, window.radarTracker extension via (window as any) - popup.js: typed getElementById casts, any-cast sendMessage responses, any-cast catch clauses; extract el() helper to avoid repetition Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…diagnostics
4.1 Health probe: probeHealth() runs against /health after Save;
result displayed as a coloured dot below the button.
4.2 Schema versioning: eventVersion surfaced in popup; transport
test asserts payload includes the field.
4.3 Drop telemetry: droppedCount from router.snapshot() shown in
status grid so silent message loss is visible.
4.4 Diagnostic export: collectDiagnostics() redacts apiKey,
bundles config + snapshot + userAgent into a downloadable JSON.
All four features covered by new regression tests (84 tests total).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add extension/README.md with full module map, storage map, schema-versioning note, and known limitations. Update CLAUDE.md extension section to reflect the new ESM/durable-queue architecture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add 5 assertion tests that protect against accidental manifest regressions: MV3 version, ESM service worker, ESM content script, permission set, and Chrome 122+ minimum. Add minimum_chrome_version: "122" to the manifest. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the MV3 extension into a modular ES‑module architecture with a durable alarm-driven send pipeline, configuration/health UX improvements, and CI/type-safety enforcement around manifest and lib modules. Sequence diagram for popup Save with health probe and diagnosticssequenceDiagram
actor User
participant Popup
participant Config
participant BackgroundSW
participant Router
participant RadarAPI
User->>Popup: Click Save
Popup->>Config: saveConfig(serverUrl, apiKey)
Config-->>Popup: Config
Popup->>BackgroundSW: runtime.sendMessage(type:CONFIG_UPDATED)
BackgroundSW->>BackgroundSW: forwardToContentScripts(CONFIG_UPDATED)
BackgroundSW-->>Popup: { ok:true }
Popup->>Config: loadConfig()
Popup->>RadarAPI: fetch GET serverUrl/health with Authorization
alt health ok
RadarAPI-->>Popup: HTTP 200
Popup->>Popup: update healthDot(success), healthText("OK (200)")
else health error
RadarAPI-->>Popup: non-200 or network error
Popup->>Popup: update healthDot(error), healthText(error message)
end
User->>Popup: Click Export diagnostics
Popup->>Config: loadConfig()
Popup->>Router: snapshot()
Router-->>Popup: { queueSize, droppedCount, configured, ... }
Popup->>Popup: redact apiKey, build diagnostic JSON
Popup-->>User: Trigger download whatsorga-diag-<timestamp>.json
Class diagram for new lib modules and router orchestrationclassDiagram
class UrlLib {
+string normalizeServerUrl(raw)
+boolean isValidServerUrl(raw)
}
class StorageLib {
+createStorage(area)
+get(key, defaultValue)
+set(key, value)
+remove(keys)
+clear()
}
class ConfigLib {
+string KEY
+Config defaults()
+loadConfig()
+saveConfig(patch)
+boolean isConfigured(cfg)
}
class QueueLib {
+createQueue(key, opts)
+enqueue(item)
+size()
+peek(n)
+drainHead(n)
+returnHead(items)
+clear()
+droppedCount()
+resetDroppedCount()
}
class TransportLib {
+sendBatch(serverUrl, apiKey, messages, timeoutMs, eventVersion)
}
class RetryLib {
+string ALARM_NAME
+number backoffMinutes(attempt)
+scheduleRetry(minutes)
+clearRetry()
}
class HeartbeatLib {
+runHeartbeat(serverUrl, apiKey, counts, queueSize, timeoutMs)
}
class DedupLib {
+createDedup(key, windowSize, area)
+isFresh(id)
+size()
+clear()
}
class RouterLib {
+createRouter()
+acceptBatch(messages)
+retryNow()
+snapshot()
+clear()
}
class BackgroundJS {
+createMessageHandler()
+forwardToContentScripts(msg)
+bumpHeartbeatCount(chatId)
}
class PopupJS {
+applyServerForm(serverUrl, apiKey)
+probeHealth(cfg)
+collectDiagnostics()
}
RouterLib --> ConfigLib : uses
RouterLib --> QueueLib : owns
RouterLib --> TransportLib : uses
RouterLib --> RetryLib : uses
ConfigLib --> StorageLib : uses
QueueLib --> StorageLib : uses
DedupLib --> StorageLib : uses
HeartbeatLib --> TransportLib : uses HTTP
PopupJS --> ConfigLib : loadConfig/saveConfig
PopupJS --> RouterLib : snapshot
BackgroundJS --> RouterLib : createRouter
BackgroundJS --> HeartbeatLib : runHeartbeat
BackgroundJS --> ConfigLib : loadConfig
BackgroundJS --> DedupLib : MESSAGE_CAPTURED counts via storage
BackgroundJS --> RetryLib : uses ALARM_NAME
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
router.clear()you callqueue.clear()but never reset the dropped counter, even thoughcreateQueueexposesresetDroppedCount(); consider invoking it there so the "Dropped" metric in the popup reflects the cleared state. - In
background.jsthe alarm handler constructs a newcreateRouter()instance on every tick (and also inside the heartbeat branch); you could reuse the existing router created increateMessageHandler()to avoid redundant instantiation and keep all routing logic going through a single instance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `router.clear()` you call `queue.clear()` but never reset the dropped counter, even though `createQueue` exposes `resetDroppedCount()`; consider invoking it there so the "Dropped" metric in the popup reflects the cleared state.
- In `background.js` the alarm handler constructs a new `createRouter()` instance on every tick (and also inside the heartbeat branch); you could reuse the existing router created in `createMessageHandler()` to avoid redundant instantiation and keep all routing logic going through a single instance.
## Individual Comments
### Comment 1
<location path="extension/popup.js" line_range="110-115" />
<code_context>
+ // Diagnostic export
+ el('diagBtn').addEventListener('click', async () => {
+ const diag = await collectDiagnostics();
+ const blob = new Blob([JSON.stringify(diag, null, 2)], { type: 'application/json' });
+ const url = URL.createObjectURL(blob);
+ const a = document.createElement('a');
+ a.href = url;
+ a.download = `whatsorga-diag-${diag.timestamp}.json`;
+ a.click();
+ URL.revokeObjectURL(url);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using ISO timestamps directly in filenames will introduce illegal characters (e.g. ':' on Windows).
Consider normalizing the timestamp to a filesystem-safe form before using it in the filename, e.g.:
```js
const safeStamp = diag.timestamp.replace(/[:.]/g, '-');
a.download = `whatsorga-diag-${safeStamp}.json`;
```
This will keep the exported file usable across platforms.
</issue_to_address>
### Comment 2
<location path="extension/src/lib/router.js" line_range="94-39" />
<code_context>
+ async size() {
+ return ((await store.get(key, [])) || []).length;
+ },
+ async clear() {
+ await store.set(key, []);
+ },
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CLEAR_QUEUE does not reset the droppedCount metric, which may confuse the popup’s "Dropped" indicator.
`clear()` only empties the stored queue; it doesn’t reset `queue.droppedCount()`, which the popup uses. After “Clear queue”, the UI will still show the old dropped total. Either reset the dropped counter here (e.g. `await queue.resetDroppedCount();`) so “Clear queue” truly clears state, or clarify in the UI label that the dropped count is lifetime/session-based rather than tied to the current queue contents.
Suggested implementation:
```javascript
async size() {
return ((await store.get(key, [])) || []).length;
},
async clear() {
await store.set(key, []);
if (typeof this.resetDroppedCount === 'function') {
await this.resetDroppedCount();
}
},
```
This change assumes that the queue object already has a `resetDroppedCount()` method or that you will add one alongside `droppedCount()` (and that it resets whatever storage the popup reads from). If `resetDroppedCount()` does not exist yet, you should implement it on the same queue object so it zeroes out the metric used by `queue.droppedCount()`, ensuring the popup’s “Dropped” indicator matches the cleared state.
</issue_to_address>
### Comment 3
<location path="extension/src/lib/heartbeat.js" line_range="13-14" />
<code_context>
+ if (!serverUrl || !apiKey) return { sent: [], remaining: { ...counts }, skipped: 'not_configured' };
+
+ const entries = Object.entries(counts).filter(([, n]) => n > 0);
+ const remaining = { ...counts };
+ for (const [chatId] of entries) remaining[chatId] = counts[chatId];
+
+ const sent = [];
</code_context>
<issue_to_address>
**nitpick:** The initialization of `remaining` redundantly reassigns the same values, which can be simplified.
Because `remaining` already copies `counts`, the loop
```js
for (const [chatId] of entries) remaining[chatId] = counts[chatId];
```
never changes any values and can be removed. The later `Object.keys(remaining)` cleanup still handles zero counts, so behavior remains the same while simplifying the code.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+110
to
+115
| const diag = await collectDiagnostics(); | ||
| const blob = new Blob([JSON.stringify(diag, null, 2)], { type: 'application/json' }); | ||
| const url = URL.createObjectURL(blob); | ||
| const a = document.createElement('a'); | ||
| a.href = url; | ||
| a.download = `whatsorga-diag-${diag.timestamp}.json`; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using ISO timestamps directly in filenames will introduce illegal characters (e.g. ':' on Windows).
Consider normalizing the timestamp to a filesystem-safe form before using it in the filename, e.g.:
const safeStamp = diag.timestamp.replace(/[:.]/g, '-');
a.download = `whatsorga-diag-${safeStamp}.json`;This will keep the exported file usable across platforms.
| }); | ||
| if (result.outcome === 'ok') return { outcome: 'ok' }; | ||
| if (result.outcome === 'auth_error') { | ||
| await clearRetry(); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): CLEAR_QUEUE does not reset the droppedCount metric, which may confuse the popup’s "Dropped" indicator.
clear() only empties the stored queue; it doesn’t reset queue.droppedCount(), which the popup uses. After “Clear queue”, the UI will still show the old dropped total. Either reset the dropped counter here (e.g. await queue.resetDroppedCount();) so “Clear queue” truly clears state, or clarify in the UI label that the dropped count is lifetime/session-based rather than tied to the current queue contents.
Suggested implementation:
async size() {
return ((await store.get(key, [])) || []).length;
},
async clear() {
await store.set(key, []);
if (typeof this.resetDroppedCount === 'function') {
await this.resetDroppedCount();
}
},This change assumes that the queue object already has a resetDroppedCount() method or that you will add one alongside droppedCount() (and that it resets whatever storage the popup reads from). If resetDroppedCount() does not exist yet, you should implement it on the same queue object so it zeroes out the metric used by queue.droppedCount(), ensuring the popup’s “Dropped” indicator matches the cleared state.
Comment on lines
+13
to
+14
| const remaining = { ...counts }; | ||
| for (const [chatId] of entries) remaining[chatId] = counts[chatId]; |
Contributor
There was a problem hiding this comment.
nitpick: The initialization of remaining redundantly reassigns the same values, which can be simplified.
Because remaining already copies counts, the loop
for (const [chatId] of entries) remaining[chatId] = counts[chatId];never changes any values and can be removed. The later Object.keys(remaining) cleanup still handles zero counts, so behavior remains the same while simplifying the code.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d23d0adaa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+23
to
+24
| const stored = await store.get(KEY, {}); | ||
| return { ...defaults(), ...stored }; |
There was a problem hiding this comment.
loadConfig() now reads only whatsorga_config_v1, so users upgrading from the previous release (which stored serverUrl, apiKey, whitelist, and enabled as top-level chrome.storage.local keys) are treated as unconfigured until they manually re-enter settings. In production this causes background routing to queue everything (not_configured) immediately after update, even though valid settings already exist in storage.
Useful? React with 👍 / 👎.
Comment on lines
+89
to
90
| await saveConfig({ whitelist: [...list, name] }); | ||
| notifyConfigUpdated(); |
There was a problem hiding this comment.
The popup now saves whitelist/enabled through saveConfig() (single whatsorga_config_v1 object), but the content script still loads whitelist and enabled from legacy top-level keys in loadConfig() (content.js). After adding contacts or toggling capture in the popup, the content script continues using stale values, so fresh installs can remain effectively non-whitelisted and stop capturing expected chats.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Chrome extension to MV3-friendly durability by moving message delivery to a chrome.storage.session-backed queue with chrome.alarms retries, while extracting core logic into testable ESM modules and adding CI-style invariants/tests.
Changes:
- Introduces modular
src/lib/*ESM utilities (config/storage/url/queue/transport/retry/heartbeat/router) and refactors background/content/popup to use them. - Replaces the old in-page queue manager with an alarm-driven durable retry queue designed to survive MV3 service-worker suspension.
- Adds a comprehensive Vitest suite (unit + integration) and manifest invariants checks; updates manifest for
minimum_chrome_versionand module scripts.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/manifest.json | Pins Chrome minimum version, converts SW + content script to ESM modules, removes legacy queue-manager script |
| extension/background.js | New thin MV3 dispatcher using router/retry/heartbeat modules + alarms |
| extension/content.js | Switches dedup to lib module; routes sends to background router |
| extension/popup.js | Switches to lib-backed config + adds health probe + diagnostic export |
| extension/popup.html | Adds health indicator, schema version, dropped counter, diagnostic export button |
| extension/src/lib/*.js | New core modules for URL/config/storage/queue/transport/retry/heartbeat/router |
| extension/tests/** | New tests for manifest invariants + lib modules + integration flows |
| extension/tsconfig.json | Removes deleted legacy queue-manager entry from compilation inputs |
| extension/README.md / CLAUDE.md | Documents new architecture, storage map, limitations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
63
to
67
| async loadConfig() { | ||
| const data = await chrome.storage.local.get(['whitelist', 'enabled']); | ||
| this.whitelist = data.whitelist || []; | ||
| this.enabled = data.enabled !== false; | ||
| console.log(`[Radar] Config: enabled=${this.enabled}, whitelist=[${this.whitelist.join(', ')}]`); |
There was a problem hiding this comment.
loadConfig() still reads chrome.storage.local keys whitelist and enabled, but the popup/background have moved config persistence to the single whatsorga_config_v1 object via src/lib/config.js. As a result the content script won’t see whitelist/enable changes from the popup (whitelist stays empty, capture never triggers). Update the content script to load these values from the new config module (or write the legacy keys as part of saveConfig for backwards compatibility).
Comment on lines
+55
to
+60
| async function bumpHeartbeatCount(chatId) { | ||
| if (!chatId) return; | ||
| const out = await chrome.storage.local.get([HEARTBEAT_COUNTS_KEY]); | ||
| const counts = out[HEARTBEAT_COUNTS_KEY] || {}; | ||
| counts[chatId] = (counts[chatId] || 0) + 1; | ||
| await chrome.storage.local.set({ [HEARTBEAT_COUNTS_KEY]: counts }); |
There was a problem hiding this comment.
bumpHeartbeatCount does a non-atomic read-modify-write on chrome.storage.local. If multiple MESSAGE_CAPTURED events arrive close together, increments can race and some counts can be lost. Consider serializing updates (mutex/promise chain) or batching counts in-memory and flushing on the heartbeat alarm.
Comment on lines
+85
to
86
| <button id="diagBtn" class="btn btn-small">Export diagnostics</button> | ||
| </section> |
There was a problem hiding this comment.
popup.js now contains ESM import statements, but popup.html still includes it as a classic script (no type="module"). That will cause a syntax error at runtime and break the popup UI. Load popup.js with type="module" (or bundle it to a non-module entry) so the imports work in Chrome.
Comment on lines
+27
to
+40
| async enqueue(item) { | ||
| const arr = await read(); | ||
| arr.push(item); | ||
| let dropped = 0; | ||
| while (arr.length > maxSize) { | ||
| arr.shift(); | ||
| dropped++; | ||
| } | ||
| if (dropped > 0) { | ||
| const prev = (await store.get(droppedKey, 0)) || 0; | ||
| await store.set(droppedKey, prev + dropped); | ||
| } | ||
| await write(arr); | ||
| }, |
There was a problem hiding this comment.
Queue operations (enqueue, drainHead, returnHead) do a read-modify-write sequence against chrome.storage.session without any locking/serialization. If multiple NEW_MESSAGES arrive concurrently (or an alarm-triggered drain overlaps an enqueue), writes can race and silently drop queued batches. Consider serializing all queue mutations through an internal mutex/promise chain (or adding a version/CAS mechanism) so storage updates are effectively atomic within a service-worker instance.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Owner
Author
|
Bug-fix sweep applied — addresses all 7 review findings:
3 new concurrency tests + 2 new manifest invariant tests + 1 new queue byte-cap test + 1 new router auth_error queue-state test. All 97 tests pass. |
Owner
Author
Connected-Races Sweep — 5 additional fixes on top of the original PR #10 bugfixesAfter applying the 7 findings from the code review (C1–C3, H1, M1, M2, L1), a codebase-wide audit found five more read-modify-write races on Root causeEvery Commits in this sweep
Test coverage
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chrome.storage.session) — survives MV3 service-worker suspension; messages that fail to send are no longer silently lost on worker restartchrome.alarms-based retry with exponential backoff ladder[0.5, 1, 2, 5]min, replacing the oldsetTimeoutapproach that died with the workersrc/lib/— each independently tested and coveredsrc/lib/**modulesminimum_chrome_version: "122"pinned@ts-nocheckremoved from all entry-point files; JSDoc types completeModule map
src/lib/url.jssrc/lib/storage.jschrome.storagewrappersrc/lib/config.jssrc/lib/queue.jschrome.storage.sessionsrc/lib/transport.jsfetchwrapper with timeout + classified outcomessrc/lib/retry.jssrc/lib/heartbeat.js/api/heartbeatflushsrc/lib/dedup.jschrome.storage.localsrc/lib/router.jsTest plan
npm run cipasses (lint + typecheck + manifest + 89 tests + coverage ≥ 90%)🤖 Generated with Claude Code
Summary by Sourcery
Introduce a durable, alarm-driven message delivery pipeline for the Chrome extension, refactor background/content/popup scripts onto shared ESM lib modules, and harden configuration, manifest, and diagnostics for MV3.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: